Skip to content

Strong-mode should fill omitted type parameters with type bound instead of dynamic? #26310

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
ochafik opened this issue Apr 20, 2016 · 16 comments
Assignees
Labels
legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@ochafik
Copy link
Contributor

ochafik commented Apr 20, 2016

Context: I found many usages of class Rectangle<T extends num> with the non-generic raw type Rectangle. Seems like it's treated by strong-mode as Rectangle<dynamic>, while it would seem it could / should be Rectangle<num>:

import 'dart:math';

foo(Rectangle r) => bar(r);
//                      ^
// [error] Unsound implicit cast from Rectangle<dynamic> to Rectangle<num>
bar(Rectangle<num> r) => null;

(tested with 1.16.0-dev.2.0)

@ochafik
Copy link
Contributor Author

ochafik commented Apr 20, 2016

cc/ @leafpetersen

@leafpetersen
Copy link
Member

This might be an instance of #26265 . Are you seeing this in the IDE or the command line? If in the IDE, can you wait until analysis stabilizes, edit the file in a way that causes re-analysis (say insert and remove a space from the middle of the word 'Rectangle'), and see if it fixes itself?

@leafpetersen leafpetersen added P1 A high priority bug; for example, a single project is unusable or has many test failures legacy-area-analyzer Use area-devexp instead. analyzer-strong-mode labels Apr 21, 2016
@jmesserly
Copy link

yeah and FYI, the original fix was in #25877

@ochafik
Copy link
Contributor Author

ochafik commented Apr 21, 2016

I'm seeing this in command line (dartanalyzer --strong math.dart).

Note: also happens with 1.16.0-dev.5.4, but that fix (ce806bf) seems to have made it to all 1.16.0-dev.x versions anyway.

@ochafik
Copy link
Contributor Author

ochafik commented Apr 21, 2016

If I define my own local Rectangle, it works: could it be that it treats type info from the sdk differently?

class Rectangle<T extends num> {}
// OK
foo(Rectangle r) => bar(r);
bar(Rectangle<num> r) => null;

For the record: it does know that the bound is num, as the following code fails:

import 'dart:math';
Rectangle<String> r;
//        ^
// [error] 'String' does not extend 'num'

@jmesserly
Copy link

jmesserly commented Apr 21, 2016

FYI, I'm seeing "no issues found" on your original example. Using a bleeding_edge build (864b64f)

@leafpetersen
Copy link
Member

@jmesserly can you double check? I see the issue with latest dev release, and with bleeding edge analyzer.

@jmesserly
Copy link

Oh sorry! I was hitting the bug where running from the wrong directory, --strong is ignored :(

@bwilkerson bwilkerson self-assigned this Apr 21, 2016
@bwilkerson
Copy link
Member

I can reproduce this using the command-line analyzer, but if I open the same file in my IDE (in a project that enables strong mode) I don't get an error. If I hover over either occurrence of r in the line

foo(Rectangle r) => bar(r);

it reports that r has the static type of Rectangle<num>. (Without strong mode enabled the static type is Rectangle<dynamic> as expected.)

To save time running this down, does anyone know where the substitution of the bounds for the missing type argument is happening?

@jmesserly
Copy link

To save time running this down, does anyone know where the substitution of the bounds for the missing type argument is happening?

TypeSystem.instantiateToBounds is where we handle the difference.

It needs to be called any time we want the "default instantiation behavior". For foo(Rectangle r), IIRC TypeResolverVisitor is the one that resolved parameter types?

@jmesserly
Copy link

IIRC, it should be happening on this line:

type = _typeSystem.instantiateToBounds(type);

@bwilkerson
Copy link
Member

Thanks! That's exactly where it should happen. But for reasons I don't understand yet, the type parameter for Rectangle doesn't have a bound (which explains why it isn't being used).

@jmesserly
Copy link

Oh crazy! Shot in the dark: maybe it fails to deserialize it from the SDK summary or something like that?

@bwilkerson
Copy link
Member

Thought about that. Konstantin says we're not using summaries anywhere yet. Also, I can see in the debugger where we're resolving and setting the bound (though I haven't been able to find where it's being cleared), which tells me we're not using summaries because we're following the old code path to resolve dart:math.

@bwilkerson
Copy link
Member

I believe that this is caused by a bug in the task descriptions: we're not requiring that the bounds for classes in imported libraries be resolved before resolving types in the importing library. But as Konstantin pointed out in #26265, there are also ordering issues even when the types are all defined in the same compilation unit.

@leafpetersen
Copy link
Member

Closing this as a duplicate of #26265 since I think that bug captures the issue here. If there's something additional about this bug that's not captured there, feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

4 participants