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

Summaries do not distinguish between types inferred from initializers and types inferred from class hierarchy #36210

Open
stereotype441 opened this Issue Mar 14, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@stereotype441
Copy link
Member

stereotype441 commented Mar 14, 2019

Consider this code (adapted from AmbiguousSetOrMapLiteralBothTest.test_setAndMap):

Map<int, int> map;
Set<int> set;
abstract class A {
  Map<int, int> get m1;
}
class B extends A {
  var m1 = {...set, ...map};
  var m2 = {...set, ...map};
}

Note that the initializers for both m1 and m2 both try to mix sets and maps, so this is erroneous code. But we still want to be able to give the user helpful error messages about it. In particular, for m1, we want to assume the literal is a map (due to the downward context from class A), so we want to issue an error just at the site of the spread of set. Whereas for m2, we want to issue an error for the entire literal, since there is no contextual information available to resolve the ambiguity.

That currently doesn't work, because during summary generation, both m1 and m2 are assigned the type Map<int, int> (the former because it overrides A.m1, the latter because we arbitrarily resolve ambiguous set/map literals to be maps if no other information is available). Then, during analysis of the file, the type Map<int, int> is used as the downward inference context, so both m1 and m2 are analyzed identically.

This is currently getting in the way of my implementing summary support for spread collections. In the long term, it would be nice it summaries could contain enough information to distinguish between types inferred from initializers (as in m2) and types inferred from overrides (as in m1) so that we could avoid this problem altogether.

Cc @scheglov, who is working on a replacement implementation for summaries.

@leafpetersen

This comment has been minimized.

Copy link
Member

leafpetersen commented Mar 14, 2019

the type Map<int, int> is used as the downward inference context, so both m1 and m2 are analyzed identically.

Just a quick comment based on little context, but this seems a priori dangerous. Are you sure you're not potentially changing results by having a possible incorrect downwards type? I'm thinking something like this:

T foo<T>(T x) {
  print(T);
  return <int, int>{} as T;
}

void main() {
  var x = {...foo("hello" as dynamic)};
}

where having a downwards type changes the inferred type argument to foo, and causes a downcast which wasn't there originally.

@stereotype441

This comment has been minimized.

Copy link
Member Author

stereotype441 commented Mar 15, 2019

@leafpetersen Agreed that it's a priori dangerous, which is why I filed this bug. For the short term we've altered the behavior so that ambiguous set/map literals get dynamic as their inferred type; so far in all the cases I've found this is sufficent to avoid a user-visible problem. We intend to come back and do a more thorough fix as part of @scheglov's rework of summaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.