Skip to content

Analyzer fails initializing_formal_final_test #34369

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

Closed
MichaelRFairhurst opened this issue Sep 4, 2018 · 4 comments
Closed

Analyzer fails initializing_formal_final_test #34369

MichaelRFairhurst opened this issue Sep 4, 2018 · 4 comments
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Milestone

Comments

@MichaelRFairhurst
Copy link
Contributor

MichaelRFairhurst commented Sep 4, 2018

I'm having difficulty pinning down whether this is still latest spec behavior, and what the rationale for it was. Given that neither the analyzer nor CFE pass this test, it seems like maybe it should be removed?

edit: the analyzer is incorrect here, and this test is valid. Keeping issue open for us to make a fix.

class A {                                                                        
  var x, y;                                                                      
  // This should cause an error because `x` is final when accessed as an         
  // initializing formal.                                                        
  A(this.x)                                                                      
      : y = (() {                                                                
          /*@compile-error=unspecified*/ x = 3;                                  
        });                                                                      
}

From the spec:

Initializing formals constitute an exception to the rule that every formal parameter introduces a local variable into the formal parameter scope (\ref{formalParameters}).
When the formal parameter list of a non-redirecting generative constructor contains any initializing formals, a new scope is introduced, the {\em formal parameter initializer scope}, which is the current scope of the initializer list of the constructor, and which is enclosed in the scope where the constructor is declared.
Each initializing formal in the formal parameter list introduces a final local variable into the formal parameter initializer scope, but not into the formal parameter scope; every other formal parameter introduces a local variable into both the formal parameter scope and the formal parameter initializer scope.

There are no other references to "formal parameter initializer scope," and the section on initializer lists doesn't reference scopes at all, but rather magically talks about:

For each such declaration $d$, if $d$ has the form \code{$finalConstVarOrType$ $v$ = $e$; }
then $e$ is evaluated to an object $o$
and the instance variable $v$ of $i$ is bound to $o$.

where some scope of evaluation is assumed but not expressed. Similarly, the "formalParameters" section doesn't mention anything either.

I get the feeling, best I can make of this, that the test is correct. However, I'm not confident there, and I'm suspicious of the value that this complexity in the spec adds.

@MichaelRFairhurst MichaelRFairhurst added area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test). labels Sep 4, 2018
@leafpetersen
Copy link
Member

I think this test is consistent with the resolution of #26655 .

My testing shows the CFE passing this test, but the analyzer failing it. Both implementations pass a simpler version where the initializer list is y = (x = 3).

cc @eernstg @lrhn to check me on my interpretation of #26655

@eernstg
Copy link
Member

eernstg commented Sep 5, 2018

The spec excerpt

Initializing formals constitute ... is declared.

specifies the current scope for the initializer list of a constructor with initializing formal parameters. Any expressions in the initializer list are resolved according to the current scope, and there is no need to mention at each point where an expression is considered which scope is current. That approach is used everywhere; e.g., when considering the meaning of an expression in the body of a function we're implicitly using the fact that its current scope (unless nested in something like function literal etc.) is the body scope of the function.

So x is treated like a final local variable here: It is in scope, but it only admits read access. Hence it is an error to have x = 3 in the body of the function literal in the example, as expected by the test.

.. if $d$ has the form \code{$finalConstVarOrType$ $v$ = $e$; }

This one is about initialization of instance variables whose declaration includes an initializing expression, so that's completely separate from the semantics of initializer lists. In that case the current scope is the instance scope of the class, and there is a special rule which makes it a compile-time error to reference this in such an initializing expression:

It is a compile-time error if \THIS{} appears, implicitly or explicitly, in ...
the initializer of an instance variable.

So you could say that the current scope for $e$ is "the instance scope minus this".

You could claim that we should be more forgiving in the body of a function literal which is statically guaranteed to not run before initialization list evaluation has completed (which makes it similar to expressions in the constructor body, or indeed the body of any instance member), which would mean that it wouldn't hurt to allow such a function literal body to access this, even though it occurs in an initializer list. But I don't think it's worth the trouble, developers could just move it out and make it a private instance member, and then they can freely use this in the body: y = _auxMethod.

It's possible that we can make the specification of scoping simpler and more readable, but I think the current approach is at least reasonable: Scopes are specified for each syntactic location where a term occurs for which one or more names may be looked up (e.g., all locations where an expression can occur), and the semantics of all such terms will never talk about scoping unless they introduce a new scope, they always just rely on looking up names in the 'current' scope.

For lookups, it's enough to know that we have a scope which is current for a certain location, it introduces a certain set of bindings (statically, and the dynamic semantics just needs to make it work as if we had all those scopes and looked up names lexically), and it has another specific scope as its enclosing scope. We mention each situation where a new scope is introduced, otherwise we just use the rule that when t' is a subterm of t and t doesn't introduce a new scope, t' has the same current scope as t.

@MichaelRFairhurst, does that make it more meaningful?

@MichaelRFairhurst MichaelRFairhurst changed the title initializing_formal_final_test needs triage Analyzer: initializing_formal_final_test needs triage Sep 5, 2018
@MichaelRFairhurst
Copy link
Contributor Author

@eernstg Thanks, that does help!

@leafpetersen you're right, the CFE does pass this. Not sure what test I had run that gave me the impression otherwise.

Changing this to an analyzer issue.

@MichaelRFairhurst MichaelRFairhurst added this to the PostDart2.1 milestone Sep 5, 2018
@MichaelRFairhurst MichaelRFairhurst added legacy-area-analyzer Use area-devexp instead. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test). labels Sep 5, 2018
@stereotype441 stereotype441 changed the title Analyzer: initializing_formal_final_test needs triage Analyzer fails initializing_formal_final_test Oct 21, 2018
@aadilmaan aadilmaan modified the milestones: Future, D25 Release Jun 4, 2019
@srawlins srawlins added the dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec label Jun 17, 2020
@srawlins srawlins added the P4 label Jan 11, 2021
@srawlins
Copy link
Member

Analyzer now reports an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart-model-analyzer-spec Issues with the analyzer's implementation of the language spec legacy-area-analyzer Use area-devexp instead. P4 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

5 participants