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

Add a flag to disable implicit downcasts #26583

Closed
nex3 opened this issue May 31, 2016 · 17 comments
Closed

Add a flag to disable implicit downcasts #26583

nex3 opened this issue May 31, 2016 · 17 comments
Assignees
Labels
area-analyzer P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@nex3
Copy link
Member

nex3 commented May 31, 2016

I want to ensure that there are no implicit downcasts in my code. I never intend to write them, so I expect that all instances of them are likely to be bugs. I understand that it's complex and wide-reaching to make them errors everywhere (as in #26437), but adding a flag that disables them would only affect users who enable this flag. It would probably also make it easier to determine how disruptive it would be to disable them entirely in strong mode.

@nex3 nex3 added area-analyzer analyzer-strong-mode type-enhancement A request for a change that isn't a bug labels May 31, 2016
@jmesserly
Copy link

Great idea! Yeah we should do this. For what it's worth, shouldn't be all that hard to create this flag. We already find them, more or less, but we suppress them in most cases (to make upgrading to strong mode a little easier on folks).

The hardest part is probably just adding the option to analyzer_cli and analysis_options.


Note, this wouldn't cover covariant generics, e.g. this would still be allowed at compile time:

main() {
  var ints = <int>[1, 2, 3];
  List<Object> objects = ints;
  objects.add('hello'); // runtime failure
}

We could add a flag for invariant generics as well, but that's a good deal bigger task. Also it would be a shame because we have some interfaces like Iterable<T> that are totally safe as covariant interfaces. (If we had variance annotations, we could declare it as abstract class Iterable<out T> which would restrict T to "output" positions only, therefore making it completely safe to cast Iterable<String> to Iterable<Object> for example)

@nex3
Copy link
Member Author

nex3 commented Jun 6, 2016

For my part, I'm much less worried about variance bugs than I am about other implicit downcasts.

@jmesserly
Copy link

Interesting ... it looks like this option kind of exists internally (not sure if it's exposed) as "strongModeHints", however, that will issue a hint about dynamic invokes as well, which is probably not what we want.

@jmesserly
Copy link

jmesserly commented Jun 7, 2016

A few other interesting findings:

There's something called AssignmentCast for T x = y pattern (note: it's a bit misnamed -- that is a VariableDeclaration not an assignment). Maybe we weren't sure if these downcasts should be considered explicit or implicit.

We have a special case for null. Given that null literal's static type is defined as bottom, I'm not sure how it is possible to downcast it. Maybe that dates back to our experiments with non-null primitive types.

There is also cast from dynamic, e.g. dynamic x; int y = x; ... not sure if we want to allow that. EDIT: for what it's worth C# allows dynamic to be implicitly converted to anything, according to the docs:
https://msdn.microsoft.com/en-us/library/dd264736(v=vs.100).aspx

@jmesserly jmesserly added the P2 A bug or feature request we're likely to work on label Jun 7, 2016
@nex3
Copy link
Member Author

nex3 commented Jun 7, 2016

There is also cast from dynamic, e.g. dynamic x; int y = x; ... not sure if we want to allow that. EDIT: for what it's worth C# allows dynamic to be implicitly converted to anything, according to the docs:
https://msdn.microsoft.com/en-us/library/dd264736(v=vs.100).aspx

Personally I would prefer having a warning there and having to write var y = x as int.

@leafpetersen
Copy link
Member

What about:

  --[no-]implicit-cast-on-assignment
  --[no-]implicit-cast-from-dynamic
  --[no-]implicit-cast-call-arguments

all defaulting to true (allowed). Casting from dynamic is always allowed unless that flag is set.

 --[no-]implicit-cast

defaults to true (allowed), and sets all three of the above flags.

I don't love that two of the three cover cast locations, while the third covers a specific source type. Maybe we should just implement the first and third flags, and if we get a feature request for the middle one we could add it. My sense is that if you want to avoid implicit downcasts, then implicit downcasts from dynamic are probably the most important ones to catch (since inference failures will show up as things being dynamic).

@jmesserly
Copy link

A nice, thanks for those thoughts! Based on that, I'm implementing one flag that disables all of those implicit casts.

@jmesserly
Copy link

https://codereview.chromium.org/2054443002/ starts making some progress. Assuming that approach looks good, there is follow up in analyzer_cli & analysis_options.

@jmesserly jmesserly reopened this Jun 10, 2016
@jmesserly
Copy link

oops, not actually fixed yet! My commit message incorrectly triggered auto-close.

@jmesserly
Copy link

https://codereview.chromium.org/2060013002/ - further refactoring from first CL. Changes the implicit cast feature to be better handled (directly in StrongTypeSystem assignability check). After that CL, I can work towards adding an option toCLI/server/DDC.

Bonus: it will make your DDC compilation a tad faster too! We can avoid a tree copy that adds the implicit casts.

jmesserly pushed a commit that referenced this issue Jun 13, 2016
Also changes implicit casts errors to be determined by the type system isAssignableTo code, and updates our tests to show the user-facing error levels (StaticTypeWarningCode is upgraded to errors as far as users can see, but this wasn't visible in the tests, which showed these as "warnings").

found while looking at #26583

R=brianwilkerson@google.com, leafp@google.com

Review URL: https://codereview.chromium.org/2060013002 .
@jmesserly
Copy link

Internal option is now available in Analyzer. Next step here is to add the user-facing option to analyzer_cli, analysis_server and DDC.

@jmesserly
Copy link

Going to close this as implemented, but we still need to document it and expose through the UI, along with a few other flags. That's covered in: #26782

@jmesserly
Copy link

I'm seeing strange behavior testing this out on real world code, it might be a problem in my previous CL. What is going wrong is lots of "hints" about types, for example this expression:

var lines = <String>[];
// ...
lines.map((l) => ' * $l');

produces the hint:

INFO: The argument type '(String) → dynamic' cannot be assigned to the parameter type '(String) → String'. ([dev_compiler] lib/src/closure/closure_annotation.dart:130)

Shouldn't we know that's a String -> String? It's returning a string interpolation. And why does it show up as a "hint" if we think it's a type error... ?

I'm going to investigate that further before exposing this flag.

@leafpetersen
Copy link
Member

That looks like an analyzer propagated type hint to me. Maybe something in your CL is letting a propagated type through where it shouldn't?

@jmesserly
Copy link

aha, propagated types! Forgot about those. That might be it. I don't think the implicit cast change alters any types, but it does change the StrongTypeSystemImpl isAssignableTo logic. So perhaps propagated types use that (likely in ErrorVerifier). In which case, they've been tightened as well, which reveals that we had wrong type stored on the function. At least, that's a theory.

@jmesserly
Copy link

jmesserly commented Jul 14, 2016

Reproduced with a simple test:

  void test_implicitCasts_genericMethods() {
    addFile('var x = <String>[].map((x) => "");');
    check(implicitCasts: false);
  }

Analyzer computes propagated types for lambda expressions passed to methods using _inferFunctionExpressionsParametersTypes. This happens before we've computed the strong mode staticType for the function, so it ends up storing the wrong type.

I'm tempted to skip propagated type inference for lambda expressions if in Strong Mode. It already does lambda inference, so it seems like extra work for no benefit in that case.

@jmesserly
Copy link

CL out for that: https://codereview.chromium.org/2145273002/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants