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

Would like a way to avoid raw generic types #33119

Closed
matanlurey opened this issue May 14, 2018 · 38 comments
Closed

Would like a way to avoid raw generic types #33119

matanlurey opened this issue May 14, 2018 · 38 comments
Assignees
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). customer-google3 P1 A high priority bug; for example, a single project is unusable or has many test failures strict-analysis-needed Mark issues where strict-raw-types, strict-inference and similar analysis options were helpful

Comments

@matanlurey
Copy link
Contributor

matanlurey commented May 14, 2018

Tangentially related to #31410.

While migrating swaths of internal code (probably in the tens-of-thousands-of-lines, myself), one of the most common errors users are running into when migrating to Dart2 are so-called "naked" raw types (is there a better name?). For example:

// Works as the user intended.
// List<int> x = <int>[1];
var x = [1];
// Does _not_ work as the user intended.
// List<dynamic> x = <dynamic>[1];
List x = [1];

Here is a more extreme example:

typedef Iterable<CacheEntry<K, V>> CacheEvictionFunction<K, V>(
  List<CacheEntry<K, V>> entries, 
  int entriesToBeTrimmed,
);

class CacheConfig {
  // In Dart1 this was "fine".
  // In Dart2 this fails, quite late, at runtime:
  // 
  // CacheEvictionFunction<dynamic, dynamic> "inferred"
  final CacheEvictionFunction _evictionImpl;

  CacheConfig(this._evictionImpl);
}

My 🔥 take: In Dart 2 + some flag, "raw" types that are implicitly dynamic should:

  • Be a compile-time error ("You must specify a type or ")
  • Work similar to Java's <?> (wildcard operator). That would mean:
// --no-implicit-dynamic
//
// List<int> x = <int>[1];
List x = [1];
// --no-implicit-dynamic
//
// Compile-time error: Must specify a type 'T' for List<T>
List x;
// --no-implicit-dynamic
//
// OK.
List<dynamic> x;

... if we are accepting name nominations, I nominate --strict-raw-types.

@matanlurey
Copy link
Contributor Author

Related to #32978, also.

I'd also love to see Null be an explicit type, not an inferred one.

@jonahwilliams
Copy link
Contributor

The combination of type inference and naked types is very unintuitive. It seems like "well I am providing more information to Dart, so it should infer better."

@munificent
Copy link
Member

munificent commented May 14, 2018

so-called "naked" types (is there a better name?)

Java calls them "raw types", which I hear on the Dart team sometimes.

My 🔥 take: In Dart 2 + some flag, "naked" types that are implicitly dynamic should:

Be a compile-time error ("You must specify a type or ")

I agree, it should be a compile error. We need to be clear about the several different places a raw type (or a thing that looks like a raw type) can appear. Here's the behavior I'd like:

// Error, raw type in annotation:
Set a = null;

// Error, raw type nested in annotation:
List<Set> b = null;

// No error, bare class name has type arguments inferred:
Set<int> c = new Set();

// Error, could not infer type argument for bare class name:
var d = new Set();

// Error, type inference does not fill in "half-complete" type annotations:
List<Set<int>> e = new List<Set>();

As for naming the flag, maybe --strict-raw-types?

@matanlurey matanlurey changed the title Naked type signatures are scary in Dart2 Raw type signatures are scary in Dart2 May 14, 2018
@matanlurey
Copy link
Contributor Author

@munificent Thanks. I updated my post/title to use the nomenclature "raw".

I also like --strict-raw-types more, so 👍

@jmesserly
Copy link

jmesserly commented May 14, 2018

RE:

// Does _not_ work as the user intended.
// List<dynamic> x = <dynamic>[1];
List x = [1];

If someone writes that, they can only use x as a List<dynamic>, so why would you need a more precise list type to be reified? If it's because of an implicit cast later on, that suggests the problem is really with implicit casts. Consider using --no-implicit-casts.

The rationale for preferring downwards inference type was cases like this:

List<Object> x = [1];

At the time most people wanted that to pick <Object> rather than <int>. Similarly:

List<dynamic> x = [1];

So dynamic kind of fell out of that. We also don't have a distinction between List and List<dynamic> in the language, they are equivalent.

That said I do think raw generic types are a problematic area in Dart 2, and it would be nice to find another interpretation for them.

BTW, you could use --no-implicit-dynamic. It enforces a few things like:

  • no raw generic types
  • no implicit dynamic parameters/variables
  • no implicit dynamic type arguments

We could split that up pretty easily, if some of those are easier to enforce/more urgent than others.

@jmesserly jmesserly added the area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). label May 14, 2018
@jmesserly
Copy link

CC @leafpetersen @lrhn

@matanlurey
Copy link
Contributor Author

@jmesserly:

RE:

// Does _not_ work as the user intended.
// List<dynamic> x = <dynamic>[1];
List x = [1];

If someone writes that, they can only use x as a List<dynamic>, so why would you need a more precise list type to be reified? If it's because of an implicit cast later on, that suggests the problem is really with implicit casts. Consider using --no-implicit-casts.

That might "fix" the problem, in that, the user will know they are likely making a mistake, but I don't see any reason to treat List (raw) as List<dynamic>. It's just doesn't work in Dart2 almost at all, so it seems like a bug not a feature.

I think --strict-raw-types should either change the semantics or raw types, or ban them.

The rationale for preferring downwards inference type was cases like this:

List<Object> x = [1];

At the time most people wanted that to pick <Object> rather than <int>. Similarly:

List<dynamic> x = [1];

So dynamic kind of fell out of that. We also don't have a distinction between List and List<dynamic> in the language, they are equivalent.

An explicit List<dynamic> or List<Object> makes total sense to me.

That said I do think raw generic types are a problematic area in Dart 2, and it would be nice to find another interpretation for them.

BTW, you could use --no-implicit-dynamic. It enforces a few things like:

  • no raw generic types
  • no implicit dynamic parameters/variables
  • no implicit dynamic type arguments

We could split that up pretty easily, if some of those are easier to enforce/more urgent than others.

Definitely. I don't know what process is in place for evolving the language outside of being on the language team at this point, but definitely flags around downcasts, dynamic, and raw types are really desirable. It would be nice to have an idea of where the language is going and try to create flags that guide the user towards that path.

@jmesserly
Copy link

Definitely. I don't know what process is in place for evolving the language outside of being on the language team at this point, but definitely flags around downcasts, dynamic, and raw types are really desirable. It would be nice to have an idea of where the language is going and try to create flags that guide the user towards that path.

yeah that's a good question ... IDK either. Hopefully it's on their radar. :)

At least technically speaking, it's not hard to add a flag to Analyzer/CFE for this sort of thing, if we want to experiment.

@munificent
Copy link
Member

Definitely. I don't know what process is in place for evolving the language outside of being on the language team at this point, but definitely flags around downcasts, dynamic, and raw types are really desirable.

It's still being sorted out. Right now, basically everyone is just trying to get Dart 2 out the door. Once that's settled down, @leafpetersen and @lrhn will start figuring out what the evolution process looks like.

@leafpetersen
Copy link
Member

leafpetersen commented May 15, 2018

We'll be looking at stricter lint flags for things like this. A question for @matanlurey : how do you feel about raw types with bounds? That is, if you have:

class A<T extends num> {
}

void main() {
  A x;
}

then we currently treat A as if it was A<num>. Would you want to forbid this as well, or only when it produces dynamic?

@matanlurey
Copy link
Contributor Author

@munificent:

It's still being sorted out. Right now, basically everyone is just trying to get Dart 2 out the door.

Oh sure. Just wanted to make it clear to others reading this post that this isn't any sort of formal language proposal, just a bug :)

@leafpetersen:

How do you feel about raw types with bounds?

I don't use them as much as others internally. I guess as long as the bounds is not Object/dynamic/void/Null then it might be OK.

At the same time I've been debugging issues like the following:

class ProtoContainer<T extends GeneratedMessage> {
  T data;
}

void main() {
  // In reality this isn't what users wanted, because they expect `UserMessage` or something else.
  var container = new ProtoContainer();
}

... I don't know if its possible, but maybe a way to indicate that T is a bounds, but not a default value. I know Flutter (tries) to use @optionalTypeArgs as an indicator for the reverse of this.

@matanlurey
Copy link
Contributor Author

Hit another case internally. Accidental omission of type parameters extending/implementing:

abstract class HasValue<T> {
  T get value;
}

class WizBang extends HasValue {
  @override
  String get value => 'WizBang';
}

This is totally valid code, and nothing is flagged in the analyzer or otherwise.

At runtime, trying to use a WizBang as a HasValue<String> (or similar) fails. It took quite a bit of groking to finally understand that HasValue had a type parameter T that was omitted. I'd like to see our hypothetical --strict-raw-types warn if a type with type parameters is implemented without bounds or "passing through" params:

// OK
class WizBang extends HasValue<String> {}

// OK
class WizBang<T> extends HasValue<T> {}

// Still OK, at least it is explicit
class WizBang extends HasValue<dynamic> {}

// BAD
class WizBang extends HasValue {}

@zoechi
Copy link
Contributor

zoechi commented May 25, 2018

implicit-dynamic: false produces a warning

image

Update
doesn't seem to cover all cases though.

@eernstg
Copy link
Member

eernstg commented May 25, 2018

I suspect that we have two distinct issues here, both arising from the instantiate-to-bound mechanism:

  1. Instantiate-to-bound will silently provide type arguments when a type annotation is or contains a raw type (generally, it may actually raise a compile-time error in some cases). This will yield a common supertype of all the possible regular-bounded types, but the developer may not discover that the type arguments were missing in the first place, and the chosen type may not be optimal.

  2. Instantiate-to-bound will choose the type dynamic as a default value for any missing type argument which is unconstrained (the corresponding formal type variable has no bound), and usages of a variable/parameter/returned-value/... with such a type annotation will then be subject to very forgiving static checks (e.g., xs[0].fooBar() is OK with List xs;, but not with List<Object> xs;). This choice was made based on the Dart 1 rule that missing type arguments were always dynamic, so that was the least breaking choice.

An option --no-raw-types would help developers detect and avoid situation (1), and some variant of --no-dynamic-something could be defined to catch situation (2). It all depends on whether we give a higher priority to "don't make me write & read all that type noise!" or to "don't hide the actual types from me!". ;-)

It might, however, be a good idea to make the choice of whether to rely on instantiate-to-bound or not a property of each library (or in some other way: to make it part of the source code and scope it), because otherwise we'd just force every single line of code which might ever be reused to avoid using instantiate-to-bound at all (because, if it's a build option, there would always be some important customer out there who wants to compile their programs with --no-raw-types).

@jonahwilliams
Copy link
Contributor

Another case of a Flutter user hitting this: flutter/flutter#18357

@matanlurey matanlurey changed the title Raw type signatures are scary in Dart2 Would like a way to avoid raw generic types Jun 16, 2018
@matanlurey matanlurey added analyzer-linter Issues with the analyzer's support for the linter package customer-google3 labels Jun 16, 2018
@matanlurey
Copy link
Contributor Author

matanlurey commented Jun 28, 2018

Hit a particularly nasty case internally that took 3 of us ~2 hours to resolve:

class Manager {
  Response<SubContext> getResponse() {
    return new Response<SubContext>(new SubContext(), (_) => new Response<SubContext>(_, null));
  }
}

typedef Callback<T extends Context> = Response<T> Function(T context);

class Context {}

class SubContext extends Context {}

class Response<T extends Context> {
  final T context;
  final Callback<T> callback;
  Response(this.context, this.callback);
}

void main() {
  Manager manager = new Manager();
  Response<Context> response = manager.getResponse();
  
  // Response<SubContext>
  print(response.runtimeType);
  
  // Uncaught exception:
  // TypeError: Closure 'Manager_getResponse_closure': 
  //   type '(SubContext) => Response<SubContext>' is not a subtype of type '(Context) => Response<Context>'
  print(response.callback.runtimeType);
}

See if you can spot the source of the error!

This is because raw type of Response is a Response<Context>, not a Response<SubContext>. Even though there is very judicious use of type arguments across (as far as the internal team thought), it is actually weaker than using var (using var response1 = actually fixes the bug).

Why? Because response.callback is statically saying give me a Callback<Context>, though, at runtime, we've encoded a Callback<SubContext>, and contravariance law says this is a bad. Wamp wamp.

@jmesserly
Copy link

That's a great example. However I'm not sure the problem is so much raw types, as it is final Callback<T> callback; ... callbacks like that are unsound. We're thinking about how to fix that in the type system, though.

@jmesserly jmesserly self-assigned this Jun 29, 2018
@matanlurey
Copy link
Contributor Author

Definitely. I just mean raw types also are a problem here. Use of var or Response<SubContext> would have prevented the error.

@eernstg
Copy link
Member

eernstg commented Jun 29, 2018

Just FYI, we have already taken a small step in the direction of a sound typing for expressions like response.callback.

As of bfa8be8, the language specification defines the variance of each formal type parameter of a typedef, which makes T of Callback invariant.

In other words, we've now established the foundation for knowing that Callback<S1> and Callback<S2> are unrelated types for any distinct S1 and S2 (even when S1 <: S2 or S2 <: S1). Hence the most specific expressible supertype of Callback<T> in the context of an object with static type Response<T> is Function.

As @jmesserly hinted, we may end up giving it the type Function because that's sound, but that step has not yet been taken.

With that, however, it would be safe to evaluate response.callback (because there wouldn't be any caller-side checks at all), but having static type Function it would be known at call sites to be more dangerous to call, and it would then be up to no-implicit-something style lints or options to make sure that this case is flagged for those who wish to do that.

In any case, it just doesn't match the actual situation to assume anything more specific than Function for response.callback so in that sense the 'source of the error' is that response.callback is given the static type Callback<Context>. ;-)

@matanlurey
Copy link
Contributor Author

I don't quite understand @eernstg - by Function you mean basically dynamic, right?

@eernstg
Copy link
Member

eernstg commented Jun 29, 2018

Yes, there is no static type more specific than Function which is sound for response.callback, because the enclosing class Response introduces covariance for its type argument, and the field has a type which is invariant in its type argument and uses the corresponding type variable as type argument. So we shouldn't pretend to know more.

It would be possible, and might be worth considering, to make Callback covariant rather than invariant:

typedef Callback<X> = X Function(Object);

With this, we would rely on some other check to establish the argument type (for instance, it could be a tear-off of a method with signature X Function(X) where the argument is covariant; or the check that the actual argument has type X could be written manually in the body; or maybe the function would simply tolerate arguments of other types than X such that the parameter type can be Object without needing any dynamic checks). A tear-off of a Response<C> would then have static type Callback<C>, safely.

@matanlurey matanlurey added P2 A bug or feature request we're likely to work on P1 A high priority bug; for example, a single project is unusable or has many test failures and removed P2 A bug or feature request we're likely to work on labels Aug 27, 2018
@matanlurey
Copy link
Contributor Author

This was made p1-high to reflect the priority level of the internal bug/OKR.

@jmesserly
Copy link

FYI, I recently got back from leave, so I can take a look at this.

Most of it is really straightforward except this part:

// - Work similar to Java's <?> (wildcard operator). That would mean:
//
// --strict-raw-types
//
// List<int> x = <int>[1];
List x = [1];

That's a change in semantics, so I'd like to get sign-off from the language team first. (I believe @leafpetersen is going to help with specifying the strict flags.) What I can do in the meantime is report the raw List as an error.

@matanlurey
Copy link
Contributor Author

No worries! Was just doing a bug scrub.

I think we can skip the change in semantics, and make this entirely an optional static check for now.

@jmesserly
Copy link

Sounds good!

Also, do we have another bug tracking the response.callback typing problem? That one seems to recur a lot.

I'm assuming that this bug is only about strict-raw-types.

@matanlurey
Copy link
Contributor Author

By the way, @srawlins and I would be super happy to help out, we could write some test cases, dogfood the UX messages, or whatever else you think will make rolling this out more successful.

@eernstg
Copy link
Member

eernstg commented Aug 28, 2018

// List<int> x = <int>[1];
List x = [1];

The introduction of partial type annotation inference (that is, implicitly completing List to List<int> based on the static type of the initializing expression) would be a language modification (and yet another source of complexity and confusion), so we should not go there without careful consideration.

However, linting List in the above example in order to give developers the healthy habit of never writing a half-done type annotation in the first place would be fine. That could then eliminate the confusion about whether List as a type annotation would be completed using instantiate-to-bound (to List<dynamic>), or it is subject to partial type annotation inference (yielding List<int> in the example). With partial type annotation inference we will have to get some information from elsewhere, and that might be the initializing expression for a variable declaration and the context type for the type annotation of a formal parameter of a function literal, but surely there will be other cases than that.

However, half-done type annotations would still be allowed in source code which is not subject to --strict-raw-types, and they will be completed using instantiate-to-bound, just like all other raw types where no external information is taken into account.

@jmesserly
Copy link

Do we have a preference for implementing this as a linter rule, vs adding a flag to Analyzer? (I asked a similar question on #33823, but now I'm wondering about this one too.)

@jmesserly
Copy link

I wanted to report on some initial progress: https://dart-review.googlesource.com/c/sdk/+/75629

A note of caution, I'm just starting to test this on real code. That CL is being used to gather data to inform a design document/spec. The rules implemented are subject to change.

@matanlurey
Copy link
Contributor Author

Thanks! A few questions, but this definitely looks like enough to gather data!

The type is inferred from a downwards context

What does this mean for us laypeople? :)

The type is inferred to be something other than the default type

I'm guessing this means:

class X<T> {
  X(T _);
}

void main() {
  var x = X(5); // OK
}

The class/typedef has @optionalTypeArgs from package:meta

I'm not so sure about this one. Part of the reason for this check is to avoid optional type arguments (i.e. train users not to write X when they meant X<dynamic>) - this annotation seems to go in the other direction. That being said, I don't see that annotation used that often either (~30 occurrences total in google3).

The type occurs on the right side of an is expression

That seems correct enough, unless we ever intend to make dynamic non-top.

@jmesserly
Copy link

Yeah sorry about that, these rules are still a work in progress. I'd like to send out a design document that walks through some examples. Happy to discuss here too, though.


The type is inferred from a downwards context

What does this mean for us laypeople? :)

Basically:

List<dynamic> x = [42]; // allowed

During inference, the downwards context is a type that an expression is expected to have, based on where it's used. In the example above, [42] must be assignable to List<dynamic>. So when we do inference on it, we use List<dynamic> as input to the inference algorithm. In contrast var x = [42] does not have a downwards context, and inference is based on "upwards" types, in this case int.

(Downwards and upwards refer to the direction type information is flowing through the tree. Given a simplified tree structure like this:

VariableDeclaration("x",
    InterfaceType("List", "dynamic"),
    ListLiteral(IntegerLiteral(42)))

... type information is passed "down" from the VariableDeclaration node to the ListLiteral. But if x did not have a type, we would instead pass type information "up" from the IntegerLiteral to the ListLiteral.)

The type is inferred to be something other than the default type

I'm guessing this means: [...] var x = X(5); // OK

Yup that's right. Nice example.

The class/typedef has @optionalTypeArgs from package:meta

I'm not so sure about this one. Part of the reason for this check is to avoid optional type arguments (i.e. train users not to write X when they meant X) - this annotation seems to go in the other direction. That being said, I don't see that annotation used that often either (~30 occurrences total in google3).

Right now it's only used by the always_specify_types lint, and that lint has been banned internally, because it directly contradicts the style guide. So that's why you aren't seeing it much. (always_specify_types was designed a long time ago for Dart 1, before we had type inference). Flutter folks seem to like @optionalTypeArgs; they requested it for no-implicit-dynamic (#32538).

My reason for adding it to the prototype was to give folks an "out", so they can still turn on the flag. If someone is willing to use the flag at all, it seems a lot better than them not using it :). One of the challenges we've had with our opt-in checks is: how do we get people to use them? --no-implicit-casts and --no-implicit-dynamic weren't adopted as much as we'd hoped, even though they encourage good practices.

(We may need to split up strict-raw-types too, so it's easier to adopt. strict-inference and strict-dynamic-calls should catch most of the practical issues caused by raw types, so we may be able to focus strict-raw-types on things that the other flags don't already catch.)

The type occurs on the right side of an is expression

That seems correct enough, unless we ever intend to make dynamic non-top.

Leaf reminded me of type promotion (doh!), so that rule will need to be removed.

@matanlurey
Copy link
Contributor Author

Yeah sorry about that, these rules are still a work in progress...

No worries. Thanks for clarifying!

My reason for adding it to the prototype was to give folks an "out", so they can still turn on the flag.

I agree. I think longer-term we might want to re-think (especially if the language team itself revisits what a raw type annotation means in the future), but for now the low-usage makes me think letting Flutter use this rule is fine.

We may need to split up strict-raw-types too

I could see that (/cc @srawlins), even if only to add a transitional path (it would be unfortunate to have say, 100+ flags after strict-* is implemented entirely). But I definitely like the idea of making it easy to opt-in incrementally.

@jmesserly
Copy link

Good news: my CL https://dart-review.googlesource.com/c/sdk/+/75629 is updated and sent for review. Thanks to @leafpetersen for clarifications about the desired behavior and @srawlins for the offer to help review and/or take over the CL if it needs significant changes.

@jmesserly
Copy link

We'll need to add official documentation for this, but for now the instructions are:

strict-inference and strict-raw-types can be configured via the language section of the analysis_options.yaml file, for example:

analyzer:
  language:
    strict-inference: true
    strict-raw-types: true

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Nov 3, 2021

Hi I wonder where can I know all those "stricter" checks? I have searched doc, but cannot find anything about strict-inference or strict-raw-types except this issue. So I wonder whether there are some other stricter checks (which can make my life easier)? Thanks!

@srawlins
Copy link
Member

srawlins commented Nov 3, 2021

I have a long-standing task to document these at https://dart.dev/guides/language/analysis-options.

But the list right now is just these two strict modes that you mention.

@fzyzcjy
Copy link
Contributor

fzyzcjy commented Nov 3, 2021

@srawlins Thanks!

@eernstg eernstg added the strict-analysis-needed Mark issues where strict-raw-types, strict-inference and similar analysis options were helpful label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-language Dart language related items (some items might be better tracked at github.com/dart-lang/language). customer-google3 P1 A high priority bug; for example, a single project is unusable or has many test failures strict-analysis-needed Mark issues where strict-raw-types, strict-inference and similar analysis options were helpful
Projects
None yet
Development

No branches or pull requests

9 participants