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

Should List foos = <Foo>[] infer as List<Foo>? #31865

Closed
matanlurey opened this issue Jan 11, 2018 · 20 comments
Closed

Should List foos = <Foo>[] infer as List<Foo>? #31865

matanlurey opened this issue Jan 11, 2018 · 20 comments
Labels
area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language).

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Jan 11, 2018

An internal customer ran into this pattern today:

class X {
  List foos = <Foo>[];
}

... where it is (properly, at least according to the new spec) inferring as List<dynamic>, which in turn causes runtime errors when trying to assign foos to a true List<Foo>. I understand the intention behind this:

class X {
  // Don't infer me as List<int>
  List<dynamic> anything = [1, 2, 3];
}

Though the following is less clear than me.

In my head anything should just care about being a List, but its assigned value should be of List<int>, not List<dynamic>:

class X {
  // Don't infer me as List<int>... for some reason.
  List<dynamic> anything = <int>[1, 2, 3];
}

But I really don't understand this one:

class X {
  // Inferred as List<dynamic>.
  List ints = <int>[];
}
@matanlurey
Copy link
Contributor Author

At the very least I'm looking forward to more formal documentation on the Dart website (i.e. not buried in markdown files in the SDK) that I can share with both external and internal users about the proper use of type inference and types in Dart 2.0.

@zoechi
Copy link
Contributor

zoechi commented Jan 12, 2018

Using a type annotation is to tell the analyzer to use the specified type instead of the inferred one.
A generic type T without a type annotation was always considered to be T<dynamic>.
I would find it confusing if this rule wouldn't hold anymore (or only sometimes).

I think it should be either var ints = <int>[];, then inference can take over,
or if this is not intended the correct annotation like List<int> ints = <int>[]; should be used.

@eernstg
Copy link
Member

eernstg commented Jan 12, 2018

The language team has had discussions about "partial inference" of type annotations (e.g., Iterable xs = <int>[] would infer part of the type annotation of xs and give xs the type Iterable<int>), but we haven't agreed to do it. In order to introduce this concept into the language it should be discussed and decided, so we don't have it now.

Without partial inference, omitted type arguments may be provided implicitly (as <dynamic, ..., dynamic>, which is what Dart 1 did, or based on instantiate-to-bound, which is what Dart 2 will do). For the given example, foos will still have type List<dynamic>.

@zoechi
Copy link
Contributor

zoechi commented Jan 12, 2018

When would you prefer using List foos = <Foo>[]; over var foos = <Foo>[]; and why?

Perhaps Iterable foos = <Foo>[]; is a better example?

@eernstg
Copy link
Member

eernstg commented Jan 12, 2018

I think the trade-off is that (1) var foos = <Foo>[]; in Dart 2 has an obvious semantics because the new list will be a List<Foo>, and foos will get the inferred type annotation List<Foo>; (2) List<Foo> foos = []; has an obvious semantics in Dart 2 because the type annotation is given explicitly, and inference will give priority to the type-from-context for [], and it will be a List<Foo>; but (3) List foos = <Foo>[]; incurs a loss of type information (because foos will be a List<Foo>, but we forget that and just remember List<dynamic>), and in the future it might amount to a complex setup to provide the type information (if we add partial inference, the static type of foos might depend on the type argument given on the right, and the explicit parts of the partial type annotation on the left might even influence type inference on the initializing expression on the right). So unless there is a good reason for (3) my preference would go elsewhere. ;-)

@vsmenon vsmenon added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label Jan 12, 2018
@leafpetersen
Copy link
Member

leafpetersen commented Jan 12, 2018

An internal customer ran into this pattern today:

 class X {
   List foos = <Foo>[];
 }

... where it is (properly, at least according to the new spec) inferring as List, which in turn causes runtime errors when trying to assign foos to a true List. I understand the intention behind this:

It should absolutely not be the case that this causes runtime errors when assigning foos to a List<Foo>. It might cause a static error if --no-implicit-casts is on, but the runtime type of foos here will be List<Foo> and that is absolutely runtime assignable to List<Foo>.

If you have a concrete example I can look at it.

In general:

  • If you specify type arguments for a generic method, literal, or constructor, they are obeyed.
  • Otherwise, if it occurs in a typed context then the type of the context fills in the type
  • Otherwise, if the sub-terms/arguments can be used to deduce a type then that is used
  • Otherwise, the default bounds are used

I agree that more documentation is needed, I hope we can get some breathing room for that soon.

I do have enough slides together that I could give a pretty in-depth tech talk on this next time I'm in MTV (probably early Feb), maybe we should aim for that?

@leafpetersen
Copy link
Member

To your subsequent examples, for completeness:

class X {
  // Don't infer me as List<int>
  List<dynamic> anything = [1, 2, 3];
}

Correct, this is inferred as <dynamic>[1, 2, 3].

In my head anything should just care about being a List, but its assigned value should be of List<int>, not List<dynamic>:

class X {
  // Don't infer me as List<int>... for some reason.
  List<dynamic> anything = <int>[1, 2, 3];
}

Your intuition is correct, the comment is wrong. There is no inference done at all in this example: you have specified the type of the literal, so it is obeyed.

But I really don't understand this one:

class X {
  // Inferred as List<dynamic>.
  List ints = <int>[];
}

The comment is wrong, or misleading. The static type of the variable is List<dynamic> since that is what you wrote, but the runtime type of the object assigned to the variable is List<int>, since that is also what you wrote.

@matanlurey
Copy link
Contributor Author

Got it. This makes much more sense to me now, I appreciate it.

So tl;dr, this is an implicit downcast we only catch at runtime.

@leafpetersen
Copy link
Member

So tl;dr, this is an implicit downcast we only catch at runtime.

I'm not sure which example you mean by this, but just to be 100% clear:

class X {
  List foos = <Foo>[];
}
void test() {
  List<Foo> x = new X().foos;
}

There is an implicit downcast on the assignment to x, and that downcast will succeed at runtime.

@matanlurey
Copy link
Contributor Author

Just wanted to bump this for possible priority in the next breaking release (Dart3?). I've been migrating large swaths of Dart 1 code to be compatible with Dart 2 semantics and answering questions from newbies on Stack Overflow (re: Flutter).

There is a lot of accidental dynanicism due to this pattern:

Map map = {'hello': 'world'};

User expectation is that this infers a Map<String, String>, but it doesn't, ironically it would have if they wrote var or final on the LHS instead of the type annotation.

I think we should probably make it infer by default (i.e. treat the LHS Map sort of like Java's Map<>) - you can write <dynamic, dynamic>{'hello': 'world'} if you really meant that.

It's possible to write an effective lint for this in the mean time, I assume.

@eernstg
Copy link
Member

eernstg commented Mar 16, 2018

Partial inference on type annotations could be added to the language (that would, e.g., let List denote the type List<int> in the declaration List xs = <int>[];).

I usually argue that it is too error-prone to use inference in the middle of a type annotation, and we should hence require some syntactic marker (e.g., List<> xs = ..;), so I'd like to keep the possibility on the radar that maybe we won't add it to the language.

In that case it can be addressed right now, in tools: Use var xs = e; if you want inference based on e; use Iterable<int> xs = e if you want the type annotation Iterable<int>; using Iterable xs = e; means the same thing as Iterable<dynamic> xs = e; (and in general we use instantiate-to-bound to find missing type arguments where inference does not apply), and --no-implicit-dynamic could be adjusted to flag it.

@Hixie
Copy link
Contributor

Hixie commented Mar 23, 2018

#32634 was closed as a duplicate of this, but I think it shows a more serious problem with the same underlying cause, so I'll repeat it here.

In the following example, the variable is given a partial type so that it is absolutely clear that it expects a particular kind of set. The expression then uses a factory constructor from a different type.

import 'dart:collection';

void main() {
  LinkedHashSet x = new Set<int>(); // A value of type 'Set<int>' can't be assigned to a variable of type 'LinkedHashSet'
}

It seems unambiguous that that should be a LinkedHashSet<int>, but the analyzer and the compiler fail with this. The analyzer error is above, the compiler error is:

test.dart:4:25: Error: A value of type 'dart.core::Set<dart.core::int>' can't be assigned to a variable of type 'dart.collection::LinkedHashSet<dynamic>'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.collection::LinkedHashSet<dynamic>'.
  LinkedHashSet x = new Set<int>(); // A value of type 'Set<int>' can't be assigned to a variable of type 'LinkedHashSet'
			^

@zoechi
Copy link
Contributor

zoechi commented Mar 23, 2018

@Hixie out of curiosity, what is your reason to use LinkedHashSet x = ... instead of var x = ... or final x = ...,
and how common do you think that your form is preferred over var/final?

@matanlurey
Copy link
Contributor Author

I commented on #32634, but I don't think:

LinkedHashSet x = new Set<int>()

... should be a static (or runtime) error, it's valid to assign a LinkedHashSet<int> to a LinkedHashSet<dynamic>. It looks like the CFE/Kernel process thinks that new Set must create a type of Set, but it doesn't since new Set is a redirecting factory constructor to LinkedHashSet.

@zoechi:

@Hixie out of curiosity, what is your reason to use LinkedHashSet x = ... instead of var x = ... or final x = ...,
and how common do you think that your form is preferred over var/final?

Regardless of preference, a lot of former Java programmers will write stuff like Map x = {1: 2}, not knowing this accidentally untypes the collection - in Java it's common to write Map<> and have the generics filled in, where in Dart they are always filled in as dynamic (hence this issue - I think we should change this behavior in Dart 3).

@eernstg
Copy link
Member

eernstg commented Mar 23, 2018

It is an error because it is not a downcast and not an upcast: Set<int> --> LinkedHashSet<int> --> LinkedHashSet<dynamic> goes down-then-up.

Of course, if we add partial inference on type annotations then LinkedHashSet in the type annotation would mean LinkedHashSet<int>, and it would then be a plain downcast.

But I still think it would be error-prone to allow partial type annotation inference with exactly the same syntax as instantiate-to-bound (that is: nothing), but requesting it explicitly with LinkedHashSet<> x = ...; is much better.

@Hixie
Copy link
Contributor

Hixie commented Mar 23, 2018

@zoechi I wanted to be clear that I was creating a LinkedHashSet, not just a HashSet, because the order of nodes in the set matters. I didn't, however, want to repeat the type arguments, which were (in the original code) an abomination of multiple levels of nested types. A typedef that lets me define type aliases with generics would have solved my problem as well.

In practice this was not code I would actually ship, it was just experimental code. In Flutter's codebase, we'd put all the types on both sides. But it would not surprise me to see our developers writing this kind of code, and the fact that it fails today is very sad (and a regression from Dart1).

@natebosch
Copy link
Member

I wanted to be clear that I was creating a LinkedHashSet, not just a HashSet

LinkedHashSet x = new LinkedHashSet<int>();

@zoechi
Copy link
Contributor

zoechi commented Mar 23, 2018

I still have troubles to make sense of that request.
I'd assume that one either wants to write the type on the left or on the right.
I can even understand that some want to be extra explicit and add the type on both sides,
but what I struggle with is your example where the base type is set left, but the generic type on the right.

I'd either write

var x = new LinkedHashSet<int>();

or

LinkedHashSet<int> x = new LinkedHashSet();

or if this works (only on the phone - not tested)

LinkedHashSet<int> x = new Set();

as I understood from above comments the runtime type should have the generuc type from the left side in the later 2 examples.

@Hixie
Copy link
Contributor

Hixie commented Mar 23, 2018

I like to put the type on the constructor call because that's the one that "matters", in terms of it being the actual place the type is being used. The variable is just a name for a piece of memory, the constructor is actually doing the allocation, calling the code, etc.

I like to use Set rather than LinkedHashSet because it's the closest thing to a Set literal, which is what I actually want to use. The same problem (analyzer and compiler errors) would occur with map:

LinkedHashMap x = <int, String>{};

@natebosch
Copy link
Member

The original question in this issue has been answered and we have a solution to avoid the problem raised. See #33119

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

7 participants