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

bad interaction: implicit downcast + inferred instantiated guarded type args #28646

Closed
Tracked by #28673
sigmundch opened this issue Feb 6, 2017 · 5 comments
Closed
Tracked by #28673
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).

Comments

@sigmundch
Copy link
Member

I ran into this scenario where the implicit downcast warning comes at play, but where it doesn't feel right as a user:

class A {}
class B<T extends A> {}

test() {
  Object x = new B<A>();
  B b = x;
  return b;
}

This produces a warning in B y = x:

[warning] Unsafe implicit cast from 'Object' to 'B<A>'. This usually indicates that type information was lost and resulted in 'dynamic' and/or a place that will have a failure at runtime. 

I was expecting this case to be treated the same as List<dynamic> or types with no-type args: the fact that the guard is implicitly injected shouldn't add more warnings to me as a developer. In fact, I also expect the runtime cost to be the same: there should be an implicit check for B, but by construction all B's are created with a type-arg that is a subtype of A, so I don't expect extra checks for the type arguments here either.

/cc @leafpetersen @vsmenon

@sigmundch sigmundch added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Feb 6, 2017
@leafpetersen
Copy link
Member

I don't understand what you expect here. In particular:

I was expecting this case to be treated the same as List<dynamic> or types with no-type args:

I think this is treated the same as List<dynamic>. What do you see as different?

In fact, I also expect the runtime cost to be the same: there should be an implicit check for B, but by construction all B's are created with a type-arg that is a subtype of A, so I don't expect extra checks for the type arguments here either.

It is certainly a valid optimization not to check the type arguments when casting to B<dynamic>, but why do you raise this here? It's an implementation issue, it doesn't affect the user facing static or dynamic semantics in any way.

@lrhn
Copy link
Member

lrhn commented Feb 28, 2017

It looks like a completely plain down-cast, which shouldn't cause a warning.
If the line B b = x; is changed to List b = x;, the warning goes away.

So, a cast from Object to B<A> causes a warning and to List<dynamic> does not - that sounds like a bug. Either is a perfectly valid down-cast.

@leafpetersen
Copy link
Member

Ah, fair enough. I wasn't factoring in the bound. This warning is going away anyway ASAP (#28588), except possibly as a lint. We should reconsider exactly what the lint should be, but at least for now that's a linter issue and now a language issue.

@sigmundch
Copy link
Member Author

Thanks @lrhn for clarifying - that's exactly the underlying issue. My claim here is that even if we had the downcast-composite warning, we shouldn't show it in cases where the type argument is a bound.

So if something is added in the linter regarding downcast-composite, this is one case that I believe should be excluded or treated differently.

@leafpetersen
Copy link
Member

So, a cast from Object to B causes a warning and to List does not - that sounds like a bug. Either is a perfectly valid down-cast.

No, this is the point of the downcast composite warning. Downcast composite was added to try to point out casts that might pass on the VM/dart2js and fail on DDC. There are no types such that the cast from Object to List<dynamic> would pass on non-strong platforms but fail on strong platforms. Ignoring the bound, there are types such that the cast from Object to B<A> will pass on the VM/dart2js but fail on DDC (specifically, B<dynamic>). So, again, ignoring the bound, the warning is working as intended.

Siggi's point is fair, that on DDC we will prevent you from creating a B<dynamic> in the first place, so in fact that cast isn't dangerous. So it's a reasonable feature request, to factor in the bound into the reasoning for that.

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).
Projects
None yet
Development

No branches or pull requests

3 participants