Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Representation variable of an extension type is not promoted #53446

Closed
Tracked by #49732
sgrekhov opened this issue Sep 6, 2023 · 19 comments
Closed
Tracked by #49732

Representation variable of an extension type is not promoted #53446

sgrekhov opened this issue Sep 6, 2023 · 19 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-feature-extension-types Implement extension types feature in the CFE feature-extension-types Implementation of the extension type feature P2 A bug or feature request we're likely to work on

Comments

@sgrekhov
Copy link
Contributor

sgrekhov commented Sep 6, 2023

[Edit eernstg: See https://github.com/dart-lang/language/issues/3342 as well.]

According to the extension type specification representation variable is subject to promotion. But in the current implementation nor analyzer nor CFE don't promote it. For example, they both report errors below

// SharedOptions=--enable-experiment=inline-class

extension type ET1(int? id) {
  void test() {
    if (id != null) {
      id.isOdd; // Error
    }
  }
}

extension type ET2<T>(T id) {
  void test() {
    if (id is int) {
      id.isOdd; // Error
    }
  }
}

main() {
  ET1 et1 = ET1(1);
  if (et1.id != null) {
    et1.id.isEven; // Error
  }

  ET2<num?> et2 = ET2(2);
  if (et2.id != null) {
    et2.id.isEven;  // Error
  }
}

A separate issue for private fields promotion #53439

Tested on the edge SDK (Sept 6, 2023) on Linux x64

@mit-mit
Copy link
Member

mit-mit commented Sep 6, 2023

@eernstg can you comment ?

@lrhn
Copy link
Member

lrhn commented Sep 6, 2023

The specification does say that.
It doesn't say precisely which kind of promotion is allowed.

I'll assume that it is only something like this-promotion, allowed inside methods of the same declaration.

But we don't have this-promotion, not even in extension members, so I'm not convinced we should have representation object variable promotion at all. It feels like a weird exception to treat that as a promotable local variable, when we don't even for this.

For field-promotion (mk. 2), a representation object accessor is definitely stable, so it can safely be promoted and used as a receiver for field promotion, but only inside the declaring library.

@a-siva a-siva added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Sep 6, 2023
@eernstg
Copy link
Member

eernstg commented Sep 6, 2023

The specification doesn't spell out the details, but it has been part of the proposal for a long time that the representation variable can be promoted because it's stable (it cannot be overridden, and it is final).

So the intention is that it should be promotable. However, it hasn't been spelled out in detail.

Final private instance variables are currently (3.2) only promotable when they are accessed via this or via a promotable local variable, and (because they are private) only in the same library.

So let's start with that: Promotion only occurs for a representation variable it in the same library as the one that declares the extension type, and only when it is accessed using this.it, or using e.it where e is a stable expression. A stable local variable is a local variable which is final, or which is stable (not mutated) according to the flow analysis. A stable expression is a stable local variable, or it is of the form e.it where e is a stable expression and it is a representation variable, or of the form e._x where _x is a promotable private final class instance variable.

@srawlins
Copy link
Member

@johnniwinther @scheglov would it be easiest if I split this into an issue per front-end?

@scheglov
Copy link
Contributor

Probably not. I suspect that promotion will be implemented in shared code.
If we will do it at all.
I see that there are discussions about stable getters in the language repository, don't know if there is something planned.

@srawlins
Copy link
Member

I think when @eernstg writes above:

So let's start with that: Promotion only occurs for a representation variable ...

I think he means that this is to be taken as part of the spec today, not a future plan as part of a separate language feature.

@scheglov
Copy link
Contributor

We don't have such word as "stable" in the specification yet.

But it seems reasonable to me that we say that it is promotable because it cannot it overridden, and it is final.
So, maybe this is just a tiny extension on top of the existing promotion for private final fields?

What I don't understand, is the limitation "in the same library as the one that declares the extension type".
Could you explain?

@lrhn
Copy link
Member

lrhn commented Sep 11, 2023

Currently field promotion only applies to field declared inside the same library, for a number of reason. The main reason is that it makes a final field behave differently from a getter, and outside of the library, there should be no distinction. Whether a getter is written as a get declaration or is the implicit instance variable getter is not part of the interface, and should be able to vary over time without it breaking anyone.
Inside the same library, we consider it OK to depend on the implementation details, since it's your own details, and if you break anything, you'll notice immediately and be able to fix it.

I want "extension type representation variable promotion" to also only apply inside the library where the extension type is declared. Whether an extension type getter is declared as by a get declaration or by being the extension type representation variable is an implementation detail, which can change over time. Nobody else should be able to depend on one particular getter being special.

So if (ext.foo != null) ext.foo.someMethod(), where ext has an extension type and foo is its representation variable, should only promote ext.foo if done inside the same libray which declared the extension type of ext.

@scheglov
Copy link
Contributor

OK, now I understand, thank you.

It is easy to restrict to a single library.
Although I don't quite agree with this motivation.

We develop in Dart on the level of packages, not libraries. I know that the language does not tell anything about packages, but they are the fact of life. It seems natural to declare some extension types in one library, to be used in another library.

@lrhn
Copy link
Member

lrhn commented Sep 12, 2023

This particular promotion could probably be allowed for an entire package.
We'd have to specify what "package" means to the language, which currently doesn't have any such notion, but it should be possible.

The other field promotions depend on knowing something about the entire program, which is different from knowing the entire package, since the package can contain more files than what are actually in the program. Or each program, when every test is its own program.
Also, it's based on library private names.

@eernstg
Copy link
Member

eernstg commented Sep 12, 2023

Sorry, this kind of promotion isn't sound after all:

extension type E(num n) {
  void foo() {
    if (n is int) {
      e = E(1.5);
      n.isEven;
    }
  }
}

E e = E(1);

void main() {
  e.foo();
}

The point is that assignment to a variable whose type is an extension type amounts to the same thing as assignment to the representation variable.

So the representation variable may be considered to be final according to all usages inside the extension type (in particular, n = 1.5 is a compile-time error in the example above), but for purposes like promotion it must be considered to be as mutable as the storage location of the "enclosing extension typed object".

@eernstg
Copy link
Member

eernstg commented Sep 12, 2023

Cf. dart-lang/language#3339.

@lrhn
Copy link
Member

lrhn commented Sep 12, 2023

Assigning to an extension-typed variable should count as assigning to another receiver, in that it can change the value of all final variables accessed through that receiver.
it's not as much assigning to the representation variable, as it is assigning to a variable that used to hold the receiver for a call.

For:

extension type E(num n) {
  void foo() {
    if (n is int) {
      e = E(1.5);
      n.isEven;
    }
  }
}

E e = E(1);

void main() {
  e.foo();
}

that code should work and be sound, even if we allow promotion of n when accessed through this.
Accesses to the representation variable through this (which is the representation object) is unaffected by assignment to the variable that value was read from.
The n.isEven should (must!) run on the same binding of this/n as the rest of the method, which is the integer 1. Assigning to the unrelated variable e doesn't change that.

@eernstg
Copy link
Member

eernstg commented Sep 12, 2023

that code should work and be sound, even if we allow promotion of n when accessed through this.

Ah, you're right, my thinking went off track!

The value of this as well as the value of n during an execution of foo is a reference to the representation object. This is definitely what we have specified.

(The representation variable, as well as this, might actually be implemented as a parameter of a top-level function which is the result of desugaring the extension type method. In a sense we do have a wrapper object that contains a reference to the representation object, but it is actually inlined into the activation record that supports the execution of foo. And nobody can assign to a parameter of a running function invocation from the outside.)

OK, so we're back to square two: We were in the middle of a discussion about the promotion which is now (again ;-) known to be sound: In which situations do we allow it?

I was proposing that we should adopt rules that are similar to the rules that we currently have for promotion of instance variables: Allow the promotion, but only in the same library.

@eernstg
Copy link
Member

eernstg commented Sep 12, 2023

I created dart-lang/language#3342 in order to settle the question about promotability of representation variables.

@pq pq added the P2 A bug or feature request we're likely to work on label Sep 25, 2023
@johnniwinther johnniwinther added the cfe-feature-extension-types Implement extension types feature in the CFE label Oct 13, 2023
@scheglov
Copy link
Contributor

scheglov commented Nov 7, 2023

Implemented in the analyzer as 6968295.
So, this code works.

extension type A(num _it) {
  void foo() {
    if (_it is int) {
      _it.isEven;
    }
  }
}

The original code above does not work because it uses not private field as representation.

@lrhn
Copy link
Member

lrhn commented Nov 9, 2023

The non-private field should work as well, I think. Since the getter is statically resolved, it's impossible for there to exist a non-stable override.

(We really should allow promotion of all final static variables inside the same library too, not just private instance fields. @stereotype441)

@eernstg
Copy link
Member

eernstg commented Nov 9, 2023

Promoting the representation variable in all cases (private or not) was part of an earlier version of the spec, and when the language team thought that was too aggressive I proposed that we should allow promotion of all representation variables in the same library. However, the language team only wanted to support promotion of representation variables with a private name, such that the rules are similar to the rules about promotion of instance variables. Here is a comment that I wrote on that same issue in order to summarize the discussion in the language team.

The spec change (that is, the latest spec change about this topic) was made here.

@johnniwinther johnniwinther self-assigned this Nov 10, 2023
@lrhn
Copy link
Member

lrhn commented Nov 10, 2023

To bad. Good thing all my representation variables are named _.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. cfe-feature-extension-types Implement extension types feature in the CFE feature-extension-types Implementation of the extension type feature P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

9 participants