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

Allow read-only access to initializing formals. #26655

Closed
5 tasks done
eernstg opened this issue Jun 8, 2016 · 23 comments
Closed
5 tasks done

Allow read-only access to initializing formals. #26655

eernstg opened this issue Jun 8, 2016 · 23 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...).

Comments

@eernstg
Copy link
Member

eernstg commented Jun 8, 2016

For convenience, and because it is non-disruptive, we wish to support read-only access to initializing formal arguments, that is, the this.x style arguments that generative constructors may have. An example:

class C {
  final int x;
  final int y;
  C(this.x) : y = x + 1 {
    int z = x + 2;
    assert(z == y + 1);
  }
  const C.constant(this.x) : y = x + 1;
}

The details of this language change are as follows:

  • The grammar is unchanged.
  • An initializing formal this.x of a constructor C introduces the name x into a scope that covers C's initializers, but not the body; it is considered to be a final parameter.
  • The semantics of such an initializing formal access is that it accesses the parameter, not the field. This matters because the initializing formal may be captured by a function expression, and the field may be updated.

The feature is currently available in dart2js under the flag --initializing-formal-access (which will be removed such that the feature is available in general), and some tests are available in the files tests/language/initializing_formal_*_test.dart.

Tracking bugs for the individual tasks:

There are no tasks for the formatter and style guide because this change does not affect the syntax.

Edit: Note that the scope rule was changed such that the initializing formal is not in scope in the body, which means that existing usages of the field in the body will preserve their current semantics.

@floitschG floitschG added the area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). label Jun 8, 2016
@kevmoo kevmoo added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Jun 8, 2016
@munificent
Copy link
Member

munificent commented Jun 8, 2016

such that x can be accessed in C's initializers and body; it is considered to be a final parameter.

I thought the plan was that the initializing formal is only visible in the initialization list. Putting it in scope in the body would shadow the field, which I think is a breaking change. Consider:

class C {
  String x;
  C(this.x) {
    x = "after";
  }
}

main() {
  var c = new C("before");
  print(c.x);
}

This prints "after" today but would be an error with the plan here, right?

@floitschG
Copy link
Contributor

correct.

@mhausner
Copy link
Contributor

mhausner commented Jun 8, 2016

@floitschG: can you clarify what you mean by "correct"?

Shadowing the field in the constructor body (as described by @eernstg above) is indeed a breaking change, but it's the only reasonable semantics. Magically hiding the initializing formal parameter in the body would go against all scoping rules in Dart.

We do have code in the dart library that breaks under the new semantics. For example:

class _FileStream ... {
 ...
  _FileStream(this._path, this._position, this._end) {
    if (_position == null) _position = 0;  // <<<< this will be a runtime error.
  }
  ...

@munificent
Copy link
Member

munificent commented Jun 8, 2016

it's the only reasonable semantics. Magically hiding the initializing formal parameter in the body would go against all scoping rules in Dart.

From the spec's perspective, an initializing formal creates a local variable whose scope is the initializer list and which disappears before the constructor body, because, I think, that's the easiest way to spec it.

From the user's perspective, though, what they will think they see is that the field is in scope during the initialization list (but can only be read from and not written to). Of course, that's not strictly true, because the "field" is accessible even before the super() call which allocates the instance, but that's a detail most users can ignore without noticing a difference.

What the user will see is what they expect to see, I think: a name that comes into scope at the constructor parameter list and stays in scope through the body. It's just that half-way through, it magically transforms from a read-only local variable to the actual field, in a way that's virtually invisible to them.

@eernstg
Copy link
Member Author

eernstg commented Jun 9, 2016

I think magically switching x from being a denotation of a parameter in the initializer list into being a denotation of the field in the body would be a really nasty trap for people to fall into (in the rare cases where it matters they would fall right into it and spend a long time discovering what's going on). It's a very alien kind of scoping.

The main usage where it matters (which is already a bit weird) is probably receiving the value of a field directly by an initializing formal, then later fixing it up to be something else in the body. Why not just receive the value as an explicit parameter and then compute the right value for that field in an initializer or in the body? But that case would be caught, because it is modifying a final parameter.

A more tricky case is when the body contains a function literal that captures the field (now: the parameter), and the field gets modified later on. That's semantically breaking, and it is likely to go under the radar. Could we hint that this is a dangerous practice when we discover that the capture of an initializing formal parameter occurs? Is it ever likely to be exactly the right thing to do, or can we assume that "they should have captured the field, anyway" is always true?

In all cases the problem can be handled by changing occurrences of x in the body to this.x, which would correspond to the usage which is probably already common when a normal parameter has been declared with the same name as a field, and both the parameter and the field are accessed in the body.

To sum up, this is very much about detection of the breaking constructs, and we do have information about where they are at compile time.

@kmillikin
Copy link

On Thu, Jun 9, 2016 at 12:05 AM Bob Nystrom notifications@github.com
wrote:

it's the only reasonable semantics. Magically hiding the initializing
formal parameter in the body would go against all scoping rules in Dart.

What the user will see is what they expect to see, I think: a name that
comes into scope at the constructor parameter list and stays in scope
through the body. It's just that half-way through, it magically transforms
from a read-only local variable to the actual field, in a way that's
virtually invisible to them.

That's not what I'd expect to see. The simple model that I would expect is
that the parameter list binds parameters, that 'this.x' is shorthand for a
parameter with the name 'x' that is used to initialize a field named x
(left to right through the iniitializing formals and before any other
initializers), and that parameter x has the exact same scoping rule as all
the other parameters
.

If I wanted to refer to the field x in the body where it's shadowed by the
parameter x, I'd expect to have to write this.x like everywhere else in
Dart. It seems like a wart that this won't be the case.

As Erik notes, you can observe the difference by capturing 'x' in an
expression in the initializer list.

@lrhn
Copy link
Member

lrhn commented Jun 9, 2016

If I wanted to refer to the field x in the body where it's shadowed by the
parameter x, I'd expect to have to write this.x like everywhere else in
Dart. It seems like a wart that this won't be the case.

It is a wart. For Dart 2.0 we might consider removing the wart and make the parameter scope cover the body.

We can't do that right now since it would break existing constructors that take a this.x parameter and refer to the x field in the constructor body using just x.

@eernstg
Copy link
Member Author

eernstg commented Jun 9, 2016

It's about breaking existing code versus having rules that make sense, i.e., rules that developers will "get" rather than constantly getting confused about them. I tend to think that "gettable" rules is a very important criterion---for instance, all parameters have the same scope rules.

Note that it's only a breaking change when the access is a capture or a modification (including x += 1), and we can give a hint about the capture.

@eernstg
Copy link
Member Author

eernstg commented Jun 9, 2016

OK, turn around, the matter has been resolved IRL: We'll let the initializing formal be in scope in the initializer list and not in the body, judging that the breakage of the rule is too heavy a burden. I'll adjust the wording of the initial comment in this issue.

@floitschG
Copy link
Contributor

For Dart 1 the final parameter will only be visible in the initializer list.
It is an observable wart, but very hard to run into. In return we have a non-breaking change that still covers 99.99% of all uses.
I will add a comment to the analyzer bug so they provide a hint/warning when users capture that variable.

@mhausner
Copy link
Contributor

mhausner commented Jun 9, 2016

Another option is to drop this language change for 1.x and do it for 2.0 only.

@a-siva
Copy link
Contributor

a-siva commented Jun 9, 2016

@eernstg I don't understand your comment stating the matter has been resolved, how was it resolved?
I agree with comment from @kmillikin that this is not what I would expect to see. Classifying it as a wart and ignoring it is not the way to proceed on this. If we want this feature in 1.x it should be done correctly even if it means it is a breaking change or like @mhausner states drop this change for 1.x

@floitschG
Copy link
Contributor

Resolution: the final parameter is only visible in the initializer list.
The initial implementation and specification was a misunderstanding.

The reason this feature is only added now, is for exactly this reason: introducing a fresh variable leads to ambiguity which variable is referenced in the body. We are adding it now, because, after writing lots of Dart code, we feel that the benefit strongly outweighs that possible confusion.

That decision hasn't changed because of the misunderstanding. It's still a feature we want to have in one of the next releases.

@kmillikin
Copy link

Then I wouldn't call it a parameter, because it isn't. It's a variable local to the initializer list.

In this code:

class C {
  var x;
  var y;
  C(x, this.x) : y = x;
}

Does the final x in scope for the parameter list shadow the parameter x? Or does the x in the nested parameter list scope prevent a declaration of x in the outer scope?

Also, why is it final? Assigning to it seems benign even if it's not necessarily useful and (most) variables in Dart are non-final by default.

@eernstg
Copy link
Member Author

eernstg commented Jun 10, 2016

The spec states that an initializing formal parameter does not introduce a name into the scope where other parameters are provided. So some choice has to be made when it is added, including how to handle duplicates. In fact, the duplicate name in C(x, this.x) is already rejected by the virtual machine (and by a new test case I created for dart2js), and it is very unlikely to break existing code.

About the scoping, I agree that the story gets a bit more complex than my initial understanding, but I don't agree that it would help developers in general to introduce an extra layer and insist that this.x can hence shadow another parameter named x.

The initializing formal parameter could be final, non-final, same-as-the-field, or controlled by an explicit final in the declaration, but given that updates to this kind of parameter are extremely likely to be bugs where the field was the intended target, making it final is an expression of the priority to make the new feature safer where the alternative would be more error-prone than useful.

@kmillikin
Copy link

kmillikin commented Jun 10, 2016

The problem with making it final is that there is no real simple workaround if that's not what is intended.

Before this change we had the equivalence: C(this.a) <==> C(x) : a = x subject to the condition that x does not occur free in the initalizers or body of C. The programmer could think of this equivalence in the right-to-left direction as a syntactic convenience for when they didn't care about the name x. And they could think of it in the left-to-right direction as the workaround they had to use when they did care about the name x.

Now, the story is not so simple and the specified behavior doesn't seem like something that can be macro-expressed in terms of existing Dart --- so we really are adding a new semantic capability to the language. And it's not one that's generally applicable, but just for this restricted situation. (There's already a simple workaround anyway, as I noted just above.)

@eernstg
Copy link
Member Author

eernstg commented Jun 10, 2016

Granted, the implicit finality is a restriction. However, usages where the intention is to modify the initializing formal argument will be new because that's a new feature. So the equivalence you mention wouldn't be used as a rewriting-rule, and in that case I believe that it would count as a reasonable workaround to declare a regular parameter and write the constructor with that in mind.

We still need a good story about how the field can be in scope in the body and not the parameter, and I'd like that story to be simple and consistent. Maybe it would work to say that there are two separate scopes for the initializer list and the body, not nested but side-by-side, and only the one for the initializer list gets the initializing formals.

As an example of a structure where it is reasonable for one construct to introduce several "sibling" scopes for its syntactic children you could consider a hypothetical typecase:

typecase (e as x) { // `x` is a fresh variable capturing the value of e for the type test.
  case int: y = x + 42; // `x` is in scope in this case, with the type int.
  case String: x += "42"; // `x` is in scope in this case, with the type String.
  ...
  default: .. // `x` might not even be in scope in the default block.
}

eernstg added a commit that referenced this issue Jun 29, 2016
In order to make an implementation of initializing formal access in
`dart2js` available as specified in issue #26655 now, this CL changes
the scope management such that initializing formals are not in scope in
the constructor body, only in the initializer list.

This is done by introducing a new notion of scopes implemented
by `ExtensionScope`: Such a scope will extend an existing
`NestedScope` rather than adding a new nested scope to it.

R=johnniwinther@google.com

Review URL: https://codereview.chromium.org/2059883002 .
@eernstg
Copy link
Member Author

eernstg commented Jul 12, 2016

The language team has created an informal language specification document pertaining to this feature which is available at https://gist.github.com/eernstg/cff159be9e34d5ea295d8c24b1a3e594.

The feature is available in dart2js under the flag '--initializing-formal-access', and the CL https://codereview.chromium.org/2141023002/ will make the effect of '--initializing-formal-access' the default behavior of dart2js. Keeping such a "make it the default" CL ready for other tools will make it possible to enable this feature by default in a coordinated manner.

@floitschG floitschG mentioned this issue Sep 1, 2016
16 tasks
@bwilkerson
Copy link
Member

  1. Is it the case that field-formal parameters are also considered to be constants for the purposes of determining whether the expressions in the initializer list are potentially constant? (It seems like it would negate the value of this change if that were not the case, so I'm guessing the answer is "yes".)
  2. The test "sdk/tests/language/initializing_formal_promotion_test.dart" appears to have an error in it, in that the x at 16:9 refers to a field, and fields are not type promoted, so the type of x at 16:21 is still B, and therefore the reference to the getter x at 16:23 would appear to require a static warning, but the test doesn't declare that a warning is expected. Did I miss something?
  3. The test "sdk/tests/language/initializing_formal_type_test.dart" appears to have an error in it, in that both the x at 13:19 and the x at 13:27 refer to a variable of type int and attempt to assign the value to a variable of type String. This would appear to require a static warning, but the test doesn't declare that a warning is expected. Did I miss something?

@lrhn
Copy link
Member

lrhn commented Sep 9, 2016

Ad 1. Absolutely. Inside the initializer list, the variable corresponding to the initializing formal should work the same as if it has been a final parameter declaration with the same value. Any difference from that is a bug in the spec. (I really want to change it to being that in Dart 2.0, but that is still contentious :).

Ad 2. It's a bug in the test.

Ad 3. Didn't miss anything. It should be a static warning and a strong-mode error (but it should work fine at runtime in spec mode).

@bwilkerson
Copy link
Member

Basic support added by https://codereview.chromium.org/2335693002/. There are probably some errors that we're not currently catching, but those should be easy to add.

The feature can be enabled using either the --initializing-formal-access command-line option, or the flag enableInitializingFormalAccess under language in the analysis options file.

@eernstg
Copy link
Member Author

eernstg commented Sep 13, 2016

That's great, thanks! The CL https://codereview.chromium.org/2332253003/ corrects initializing_formal_promotion_test to fit the current scope rules; initializing_formal_type_test cannot use multi-testing, so we need to find a different way to declare that the static warning is expected.

@eernstg
Copy link
Member Author

eernstg commented Nov 24, 2016

When this issue was created it wasn't stated explicitly when each subtask would be complete: We could say that it's complete when the feature is working and available under a flag, or we could require also the step to make it available by default. Clearly, different teams have made different choices in this respect so far.

So I've decided that this issue is considered complete for each tool when the feature is available under a flag (and I've added some checkmarks to reflect that interpretation --- please add more of them when appropriate), and I've created a new issue, #27891, to track the step where this feature is enabled by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...).
Projects
None yet
Development

No branches or pull requests

9 participants