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

Analyzer does not report an error when new is used in a non-constructor position with constructor tearoffs enabled #47077

Closed
leafpetersen opened this issue Sep 1, 2021 · 3 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on

Comments

@leafpetersen
Copy link
Member

With constructor tearoffs enabled, the analyzer correctly emits an error on the definition of the field in C in the code below, but issues no errors on the access of new on an instance of C and on foo in main. I believe that all of those should be an error. This should cause a failure in co19/Language/Expressions/Instance_Creation/New/type_t05 (source here) when constructor tearoffs are enabled. See logs here but note that the analyzer run failed with an infrastructure issue . CL turning on the flag is here.

cc @eernstg @lrhn @srawlins @scheglov @devoncarew

class C {
  int new = 42;
}

main() {
  new C().new is int;
  dynamic foo;
  foo.new;
  foo.new();
}
@leafpetersen leafpetersen added this to To do in Constructor tear-offs via automation Sep 1, 2021
@leafpetersen leafpetersen added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Sep 1, 2021
@lrhn
Copy link
Member

lrhn commented Sep 2, 2021

They should all be compile-time errors. They all use .new on something which does not denote a class, which is not valid.

For the foo.news, it's an error during resolution or static type checking, not parsing, because the grammar is potentially a constructor tearoff/invocation.

For new C().new I don't think our grammar allows it at all, but @eernstg would know for sure. (Is .new a selector, where we then reject if the receiver does not denote a class, or is it special syntax which requires the prior terms to be (prefix.)?className(<typeArgs>)??)

It doesn't really matter how the parser/compiler gets to rejecting the program, they can optimize for recovery and good error message, as long as all valid programs, and only valid programs, get through the process.

@eernstg
Copy link
Member

eernstg commented Sep 2, 2021

I believe the grammar in Dart.g, in particular the updates in 6facd6d, can be considered to specify the new syntax added by the constructor-tearoffs feature bundle. With that, we have two syntax errors (int new and new C().new), but it is of course not required for an implementation to label any error as a syntax error. I believe the behavior of dart and dartanalyzer from commit a6cc772 (a fresh one) is fine for int new, but they fail to report new C().new as an error (of any kind).

foo.new and foo.new() should be reported as compile-time errors, because foo fails to be a statically resolved reference to a class (it should be a class, and that class should have a constructor named foo, but we don't even get to step two).

Cf. dart-lang/language#1837.

@srawlins srawlins self-assigned this Sep 3, 2021
@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Sep 3, 2021
Constructor tear-offs automation moved this from To do to Done Sep 3, 2021
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. P2 A bug or feature request we're likely to work on
Projects
Development

No branches or pull requests

4 participants