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 support for Dart 1.50 non strong mode generic methods. #27641

Closed
leafpetersen opened this issue Oct 20, 2016 · 18 comments
Closed

Analyzer support for Dart 1.50 non strong mode generic methods. #27641

leafpetersen opened this issue Oct 20, 2016 · 18 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 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Milestone

Comments

@leafpetersen
Copy link
Member

We have an informal specification for Dart 1.50 generic methods here:

https://docs.google.com/document/d/1nccOSkdSyB9ls7TD8gUqIu6Wl2J_9bTSOoMCEIoAiQI/edit#heading=h.dnanv15dn1me

The analyzer should remove the partial support for error checking on generic methods (or make them into hints as warranted).

The only static errors for Dart 1.5 non strong mode generic methods are syntax errors, and errors for using a generic method parameter for an is check as in x is T, or as a type literal.

@leafpetersen leafpetersen added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Oct 20, 2016
@leafpetersen leafpetersen added this to the 1.50 milestone Oct 20, 2016
@bwilkerson bwilkerson added P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 20, 2016
@kulshekhar
Copy link
Contributor

Can the linked document be publicly shared?

@eernstg
Copy link
Member

eernstg commented Oct 21, 2016

Here's a public version of the informal specification of generic method syntax that Leaf mentioned. Note that we do not commit to the contents of this file, things may still change!

Update Nov 2nd: We are now implementing this feature, and the informal specification is not expected to change any more.

@bwilkerson
Copy link
Member

I believe that https://codereview.chromium.org/2487583002/ correctly converts errors to hints in the non-strong-mode case.

As far as I know the only remaining step is to remove the flag, so please let me know when doing so is appropriate.

@leafpetersen
Copy link
Member Author

At some point there should be a non-strong mode error when a generic method parameter is used directly in an is check (e.g. x is T). Note though that x is List<T> is not an error.

This error is just transitional, to ensure that users do not rely on instance check behavior that will differ between strong mode and non-strong mode.

@bwilkerson
Copy link
Member

Error, warning, or hint?

@leafpetersen
Copy link
Member Author

It is an error.

@leafpetersen
Copy link
Member Author

Also note, the error is only in an 'is' check, not in a cast expression ('x as T').

@eernstg
Copy link
Member

eernstg commented Nov 9, 2016

I think it makes sense to make the motivation for this treatment of is and as explicit:

With these syntax-only generic methods there is no runtime representation for type arguments declared in the signature of a function or method. So when T is such a type argument, it is inherently impossible for e is T and e is! T to evaluate to a result that depends on the actual value of T at runtime.

We could easily compile these expressions to always return true, or always return false, but in both cases it would silently introduce subtle bugs into some otherwise correct programs. Just like other boolean expressions, say x < 10, there is no "safe bet" on its value at runtime that we can choose at compile time. So the constructs e is T and e is! T are compile-time errors when T is a function type argument, because a program using them is inherently buggy (with syntax-only generic methods, but obviously they will be ok when we get full-fledged support for generic methods).

The situation is different for e as T where T is a function type argument, because there is a strong bias that such an expression in a correct program would evaluate to the value of e rather than throwing an exception. So e as T is not an error, and it evaluates to the value of e, no matter what T would have been, had it been available at runtime (and again we will get the proper check when we have full-fledged generic methods). Nevertheless, we recommend emitting a hint to inform the developer that e as T does not actually check for T, it just evaluates e.

@bwilkerson
Copy link
Member

I believe that https://codereview.chromium.org/2479293004/ adds the required error. As far as I know, that's everything except for removing the flag. Please let me know if you know differently.

@eernstg
Copy link
Member

eernstg commented Nov 9, 2016

Sounds good! I'll return with more info when we have a robust plan for how to remove the flag in a way that doesn't disrupt our internal users.

@eernstg
Copy link
Member

eernstg commented Nov 10, 2016

We've got the CLs now, for enabling this feature by default (see #27501).

@bwilkerson
Copy link
Member

Does that mean that I should commit the CL now, or are we still waiting?

@eernstg
Copy link
Member

eernstg commented Nov 10, 2016

Keep it back a little bit: We had a lot of things going on today and I haven't yet done the global pre-submit test run. I'll give a heads up when it's done, which should be tomorrow.

@dgrove
Copy link
Contributor

dgrove commented Nov 21, 2016

@eernstg can you provide an update?

@eernstg
Copy link
Member

eernstg commented Nov 21, 2016

Please check #27501 for the current status.

@eernstg
Copy link
Member

eernstg commented Nov 22, 2016

The language group discussed the situation and agreed that it is OK to proceed to land the analyzer CL and then handle the error-to-hint adjustments afterwards.

@dgrove
Copy link
Contributor

dgrove commented Nov 22, 2016

@bwilkerson when can the CL for this be landed?

cc @kevmoo

@bwilkerson
Copy link
Member

Committed

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 type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

6 participants