[Extension type] linter support #58838

pq · 2022-08-29T20:36:36Z

A meta-issue to discuss and track work on linter support for extension types.

Feature Spec

Existing Lints

Some existing lints will need tests (minimally) and possibly enhanced implementations.

🚧 Incomplete list: feedback welcome! 🚧

New Lints

Users of extension types might benefit from some new lints.

proposal: annotate_redeclares #59297

See also: #53121

...

The text was updated successfully, but these errors were encountered:

IvanDembicki · 2022-08-29T22:33:04Z

avoid_logic_in_view_classes

bwilkerson · 2022-08-29T22:48:45Z

@IvanDembicki Could you provide more information about what the proposed lint would actually do (either here or in a separate issue, the latter being my personal preference).

IvanDembicki · 2022-08-30T13:28:44Z

bwilkerson #58844

pq · 2023-08-03T17:31:58Z

@eernstg: in the spec you write:

we make no attempt to hide the representation type during the exhaustiveness analysis. The usage of such patterns is very similar to a cast into the extension type in the sense that it provides a reference of type V to an object without running any constructors of V. We will rely on lints in order to inform developers about such situations, similarly to the treatment of expressions like e as V.

Could you provide a bit of detail on the lints you had imagined?

pq · 2023-08-03T17:35:38Z

A type test, o is U or o is! U, and a type cast, o as U, where U is or contains an extension type, is performed at run time as a type test and type cast on the run-time representation of the extension type as described above.

These type casts and type tests where the target type is an extension type may be considered to "bypass" the constructors of the extension type. This proposal makes no attempt to prevent this. However, support for detecting and thus avoiding that this occurs may be provided via lints or similar mechanisms.

Perhaps one lint (no_type_extension_type_tests?) to disallow type extension type tests and casts?

pq · 2023-08-03T21:30:07Z

Proposed elsewhere:

Lint + fix to go from old style JS interop to extension types

Could someone (with more context) provide some context/examples?

eernstg · 2023-08-08T15:31:34Z

@pq wrote:

Could you provide a bit of detail on the lints you had imagined?

Sure! Ideally, they would flag any construct that performs a dynamic type test on an expression e and yields an expression whose type is an extension type which is not the static type of e nor a supertype thereof. That is, anything which is a cast or a promotion "into" an extension type.

We need to consider types where extension types occur as subterms as well. For instance, if E is an extension type then List<E> is relevant as well, and so is E Function(Map<E, String>), etc.

So let's spell out this concept in more detail:

We say that a type T contains an extension type E in the case where E is an extension type and T is a term that contains E as a subterm. We say that a type T contains a covariant extension type in the case where T contains an extension type E that occurs covariantly, and similarly for contains a contravariant extension type and contains an invariant extension type.

We say that the transition from a source type S to a destination type T introduces an extension type E if:

T contains a covariant or invariant extension type E, and S is not a subtype of T nor a supertype of T. For example, a transition from int to E, where E is an extension type with no subtype relationship to int.
T <: S, T contains a covariant or invariant extension type E, and the proof of T <: S contains a subproof of the form E <: U where U != E. For example, a transition from Iterable<dynamic> to List<E>; or a transition from Object? Function(int) to E Function(int).
T <: S, T contains a covariant or invariant extension type E, and the proof of T <: S contains a subproof where the structure of the types eliminates the type E. For instance, a transition from Object to List<E>; or a transition from Function to void Function({String Function(E) callback}).

Note that an upcast never introduces an extension type, even in cases like the transition from void Function(Object?) to void Function(E) where E is an extension type. The point is that no object will be treated as an instance of E which wouldn't have been treated as such before the transition. For example, in the simple case ((Object? o) => e) as void Function(E), the body e still treats the argument as having the type Object?. Each call site must new provide an argument with static type E, but it's a separate issue how we managed to create an expression with that type.

With that, we can list cases that are candidates to be linted:

e as T, where the transition from the static type of e to T introduces an extension type.
e is T, where the transition from the static type of e to T introduces an extension type.
on T in a try/catch statement, where T contains an extension type.
Matching against an object pattern whose type T contains a covariant or invariant extension type (in any context where a pattern can occur), in a situation where the transition from the matched value type to T introduces an extension type.
Implicit casts from dynamic to a type that contains an extension type.

Finally, the lint message should be omitted in the case where the extension type which was introduced has a direct or indirect superinterface which is a non-extension type. For example, extension type E(T t) implements S {...}, where S is a non-extension type.

You could say that implements S is a way for E to announce to the world that every object whose dynamic type is the representation type of E or a subtype thereof is an appropriate representation object for E. In contrast, an extension type that does not implement any other non-extension types than the default ones (top types for some extension types, Object for others) provide a soft kind of encapsulation in that we're not supposed to cast or promote or match our way into those extension types, and that's exactly what the lint is supposed to detect.

[Edit Oct 1 2023: The example about going from Iterable<int> to List<E> could never apply, because we wouldn't have E <: int unless the extension type had implements int, and in that case the lint would not apply anyway. So I changed the example to use dynamic rather than int, which is actually a case that could exist and would cause the lint.]

eernstg · 2023-08-09T12:11:28Z

@scheglov, do you think it would be appropriate for the analyzer to have a variant of isSubTypeOf which would return true if called with S and T such that S <: T and "the transition from T to S does not introduce an extension type"?

Perhaps it could be implemented by giving isSubTypeOf an extra optional named bool doesNotIntroduceAnExtensionType. It would then basically compute the subtype relationship, but "cancel" the subtype relationship from extension types that don't have a non-extension type superinterface other than the default one (Object or Object?) to all other types than that extension type itself. (So extension type E(T rep) {} would behave as if E <: E, but we would not have E <: Object? or E <: T for any other T which isn't E.)

This task is non-trivial, but it seems to be very similar to the computation of the subtype relationship, which means that it would be a very helpful method to have for the implementation of the "don't skip the constructor" lint which is outlined here.

pq · 2023-08-14T22:49:46Z

Regarding enforcing (CamelCase) naming, I'd love not to add another lint but would love to get people's 2 cents. How do folks feel about extending camel_case_extensions or camel_case_types to support extension types?

/cc @munificent @natebosch @eernstg @lrhn @bwilkerson @srawlins @jakemac53

parlough · 2023-08-14T22:52:29Z

I see no reason to not add it to an existing lint, at least if the lint is updated before extension types land in stable.

It seems it (and camel_case_extensions, but that's harder now) should be handled by camel_case_types to me.

pq · 2023-08-14T23:03:42Z

Thanks @parlough! I agree we may have been too conservative w/ camel_case_extensions (@bwilkerson may recall any added context) and maybe we should fold them all together down the road but yeah, that's a bit tricky...

pq · 2023-08-14T23:04:15Z

Now, if you had to choose which lint to extend?

parlough · 2023-08-14T23:10:16Z

Now, if you had to choose which lint to extend?

Unless that potential added context changes something, I would say camel_case_types. That's closer to an all encompassing term for the future, and "extension types" has "types" in it as well :)

bwilkerson · 2023-08-14T23:25:39Z

An extension type defined a type; an extension does not.

I don't remember why we created camel_case_extensions, but using camel_case_types to enforce a declaration that doesn't introduce a type might have been the motivating factor.

pq · 2023-08-25T20:33:17Z

Regarding the possible constness of the primary constructor (spec), I wonder if const-enthusiasts would value a new lint to nudge authors to declare them so.

That is:

BAD

extension type E(int i) { }

GOOD

extension type const E(int i) { }

(See also the conversation about immutability and extension types as it pertains to linting in #59275 and #59276.)

/cc @eernstg @goderbauer @a14n @bwilkerson

eernstg · 2023-08-28T09:14:08Z

As mentioned above, a primary constructor of an extension type can always be constant, because it has no initializer list, and the extension type has no non-final instance variables.

However, this nudge may be considered spammy if the advice is given for every extension type declaration, especially in cases where the static type of the parameter does not have any constant constructors (we generally can't know: except for a final type with no reopen there could be a subtype which has a constant constructor).

So it's probably good to have the lint (and have it flag all extension type constructors which can be constant, not just the primary one), but it is probably also useful to make sure that it is a separate lint for extension types, not an extension of the applicability of the corresponding lint for classes.

srawlins · 2023-09-06T18:27:54Z

A few more lint rules that need changes:

✅ avoid_types_as_parameter_types is not reporting extension types (extension type ET(int it) {} void f(ET) {}).
cancel_subscriptions seems not to report for an extension type.
collection_methods_unrelated_type seems to over-report, when a method is redeclared. E.g.
```
extension type ET(List<num> it) implements Iterable<num> {
  int contains(String i) => 7;
}
void f(ET et) {
  et.contains('1');
}
```
This should not report on the call to contains, as it has been redeclared.

discarded_futures is not reporting extention types of Future. E.g.

extension type ET(Future<int> f) {}
void f(Future<int> future) {
  g(future);
}
ET g(Future<int> future) => ET(future);

pq · 2023-09-06T21:34:06Z

Fantastic. Thanks @srawlins!

srawlins · 2023-09-11T23:28:26Z

A few more lint rules that need changes:

avoid_slow_async_io

This code should have lint reported, but does not.

import 'dart:io';
extension type ET(Directory dir) implements FileSystemEntity {}
void f(ET dir) async {
  await dir.exists();
}

unrelated_type_equality_checks (extracted issue to For unrelated_type_equality_checks (and other rules), how are extension types related to other things? #59299)

This code should not have lint reported, but does:
```
extension type ET(int it) implements num {}
void m(int? a1, ET a2) {
  var b1 = a1 == a2;
  var b2 = a2 == a1;
}
```

unsafe_html

This code should have reported lint, but does not:

import 'dart:html';
extension type ET(HeadingElement it) implements Element {}
void f(ET heading) {
  heading.setInnerHtml('<script>');
}

jacob314 · 2023-09-12T22:27:26Z

These are really interesting edge cases. It has taken me a minute to wrap my head around them. The dart:io one seems to be a generalization of a general issue where the lint doesn't consistently trigger for subclasses of File only the precise file class.

avoid_slow_async_io
This code should have lint reported, but does not.

import 'dart:io';
extension type ET(Directory dir) implements FileSystemEntity {}
void f(ET dir) async {
  await dir.exists();
}

Turns out this one is already fragile with respect to regular Dart classes. Probably a P3 bug to make it less fragile in general.

class DummyFile implements File {
  @override
  dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
}
void main() async {
  DummyFile file = DummyFile();
  await file.exists();

does not trigger the lint but

File file = DummyFile();
await file.exists();

So the existing behavior sometimes triggers on subclasses of File but sometimes doesn't.

unrelated_type_equality_checks
Here there is an interesting question of whether the lint should or should not trigger if the "implements num" clause was missing. With "implements num" the lint shouldn't fire but without it I think this is like the similar case that can occur with unrelated interfaces. Here the lint correctly triggers even though the types really are related.

class A {}

class B {}

class C implements A, B {}

void main() async {
  A a = C();
  B b = C();
  if (a == b) {
    print("equal");
  }
}

This code should not have lint reported, but does:

extension type ET(int it) implements num {}
void m(int? a1, ET a2) {
  var b1 = a1 == a2;
  var b2 = a2 == a1;
}

unsafe_html
This code should have reported lint, but does not:

import 'dart:html';
extension type ET(HeadingElement it) implements Element {}
void f(ET heading) {
  heading.setInnerHtml('<script>');
}

This lint has an existing similar issue for implementing classes. Arguably the lint should still fire for this case of a class implementing HeadingElement but it doesn't.
Example:

import 'dart:html';

class ET implements HeadingElement {
  @override
  dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
}

void f(ET heading) {
  heading.setInnerHtml('<script>');
}

srawlins · 2023-09-21T19:51:40Z

A few more lint rules that need changes:

close_sinks - This code should report a lint but does not:

import 'dart:io';
extension type E(IOSink it) {}
E get e => E(File('').openWrite());
class C {
  // ignore: unused_field
  late E _sink;
  void init() {
    _sink = e;
  }
}

avoid_setters_without_getters - I opened How should avoid_setters_without_getters handle extension types? #59307 which goes beyond what was implemented in avoid_setters_without_getters support for extension types #59264
avoid_catching_errors - This code should report a lint but does not:
```
extension type E(Error it) {}
void f() {
  try {} on E {}
}
```
only_throw_errors - I think think should not report a lint, but it does:
```
extension type E(ArgumentError it) {}
void f(E e) {
  throw e;
}
```
I guess technically an E is not a subtype of Error or Exception, but it seems like it should be fine to throw such things?

bwilkerson · 2023-09-21T20:46:22Z

This is interesting.

The avoid_catching_errors and only_throw_errors seem inconsistent. In neither case is E a subtype of Error. I think the question here is whether the lint should use the static type or the "runtime" type. We generally stick to the static type because it's knowable, but it sounds like in avoid_catching_errors case you're arguing in favor of using the runtime type. Does/should it flag

try {} on Object {}

which could also be catching Errors? Maybe extension types are special because we know more about the runtime type?

The question about close_sinks is the same. The proposal appears to be to use the runtime type rather than the static type.

If we want to go that direction, I'd want to understand better when we should use the runtime type rather than the static type so that we can be more consistent about where that technique is applied.

eernstg · 2023-09-22T09:17:37Z

Here is one way to think about the representation type of an extension type that might be useful when deciding on things like "should only_throw_errors flag throw e when the static type of e is a given extension type?":

Check whether the associated semantics includes a dynamic type test or an upcast to a top type or Object.

The point is that if the semantics of a construct discards the extension type then there is no way we can restore it (there is no evidence whatsoever for the extension type at run time). In that case it makes sense to treat the expression as if it had had the representation type as its static type (and it should be the recursively erased type that we sometimes call the 'ultimate' representation type). We're trying to take certain scenarios into account, and they may depend on the run-time type, and the ultimate representation type is our best compile-time approximation of that run-time type, but the scenarios certainly won't depend on the extension type.

For example, evaluation of throw e is linked to an upcast to Object and/or a dynamic type test, because the value of e will be handled in code where its static type is Object, or it will be subject to a dynamic type test (on T catch ...). A future which is completed with an error will also give the thrown object the static type Object.

Now what if we have the property mentioned above (i.e., the extension type is unavoidably discarded), but the most useful choice is to allow an extension type to hide the underlying representation type?

For example, what if we're creating extension type E(Error it) ... exactly because we want to catch certain errors, and we want to use the extension type for that purpose (to hide that we're catching an Error) rather than using // ignore: avoid_catching_errors again and again?

Perhaps we can come up with a scenario like this, but I tend to think that we should ignore it (and just rely on the representation type when the extension type is guaranteed to be forgotten anyway).

An interesting case is await_only_futures.

It is actually a compile-time error if we encounter await e where the static type E of e is an extension type that doesn't have implements Future<T> for any T.

So there's no need to decide on what to do in the lint if this situation arises.

If E is an extension type that does have implements ... Future<T> ... for some T then await e will already have the desired static type, so that should just work.

On the other hand, unawaited_futures should probably rely on the ultimate representation type, and flag every situation where a future is left unawaited, including cases where the unawaited object has a static type which is an extension type whose ultimate representation type would match the criterion used by unawaited_futures.

Again, the point is that we'd want to detect situations where an unexpected run-time error occurs because a future was left unawaited (and nobody was around to receive an exception), plus situations where a program has execution ordering bugs because some operations aren't awaiting the completion of other operations. In both cases, the undesirable outcome depends on the given object being a future, not on its static type.

Again, we could come up with a scenario where we want to use the extension type to hide that a certain future is a future (because we know that all these futures do not need to be awaited). However, in that case it's probably possible to avoid returning that future in the first place, or it could have the type void (not Future<void>). So I still think it makes sense to let unawaited_futures use the ultimate representation type when it encounters an extension type, no exceptions.

Otherwise, I think an extension type should probably always be preserved with respect to lints and similar checks. This supports the ability of extension types to arrange for a different treatment of the underlying object than that which is given to the representation type, and that's the main point of having extension types in the first place.

bwilkerson · 2023-09-22T14:49:31Z

Thanks, that's helpful.

I mentioned this elsewhere, but just to capture it somewhere more permanent: In order to reduce user confusion, I think it's important that we document whenever a lint will use the representation type rather than the extension type.

srawlins · 2023-09-26T00:42:44Z

unreachable_from_main needs some changes to support extension types:

extension type E(int it) {
  void m() {} // Should be reported as unreachable.
}
void main() { E(7); }

extension type E(int it) {} // Should not be reported as unreachable.
void main() { 7 as E; }

extension type E(int it) {
  E.named(this.it); // Should be reported as unreachable.
}
void main() { IntExtension(7); }

lrhn · 2023-09-26T11:34:49Z

The description of unreachable_from_main says "top level or static", and technically m in the first example is neither.
That might just be because the distinction should be between statically resolved and dynamically resolved, and the "instance members" of extensions and extension types are statically resolved, even if they are not static.
Should the description be updated?

srawlins · 2023-09-26T14:13:41Z

Good catch. I think we didn't implement for instance members to avoid complexity with inheritance. I agree with your assessment and suggestion.

Work towards https://github.com/dart-lang/linter/issues/3625 Change-Id: I41a28795f16eb19b41f864ef512cf10936c2e11f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327713 Commit-Queue: Samuel Rawlins <srawlins@google.com> Auto-Submit: Samuel Rawlins <srawlins@google.com> Reviewed-by: Phil Quitslund <pquitslund@google.com>

srawlins · 2023-10-30T18:34:32Z

@pq Should I add checklist items to the tippy top of this issue for the rules that I think need updating, which I commented on above?

pq · 2023-10-30T18:58:57Z

Thanks for bumping this, Sam. I've been meaning to! Please add items. My sense is that a bunch of the ones above are WAI as of my last conversations with @bwilkerson where I thought there was a general consensus that we did not want to lint based on runtime/representation type but I do see a few that don't fit that pattern. Thanks!

srawlins · 2023-10-30T19:12:45Z

OK, they're added. Some need work and some can probably just be checked because we decided above that I was wrong in the repro case I had written down.

pq · 2023-10-30T19:59:14Z

Ok, cool. Thanks, Sam!

pq added linter-new-language-feature P1 A high priority bug; for example, a single project is unusable or has many test failures area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). labels Aug 29, 2022

jacob314 added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Feb 14, 2023

itsjustkevin changed the title ~~☂️ linter support for views~~ ☂️[Inline Class] linter support Mar 6, 2023

itsjustkevin changed the title ~~☂️[Inline Class] linter support~~ [Extension type] linter support Jul 25, 2023

itsjustkevin mentioned this issue Jul 25, 2023

Implement Extension Type Feature #52684

Open

24 tasks

pq self-assigned this Aug 3, 2023

github-actions bot added linter-set-core linter-set-recommended labels Aug 3, 2023

github-actions bot added the linter-set-flutter label Aug 3, 2023

eernstg mentioned this issue Sep 13, 2023

Awkward two-constructor necessity in extension type with validation dart-lang/language#3343

Open

eernstg mentioned this issue Sep 26, 2023

Implicit cast-and-construct from dynamic to extension type? dart-lang/language#3369

Closed

srawlins mentioned this issue Sep 26, 2023

proposal: cast_into_extension_type #59310

Open

5 tasks

srawlins added the type-enhancement A request for a change that isn't a bug label Mar 29, 2024

devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 19, 2024

devoncarew transferred this issue from dart-lang/linter Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Extension type] linter support #58838

[Extension type] linter support #58838

pq commented Aug 29, 2022 •

edited by srawlins

Loading

IvanDembicki commented Aug 29, 2022

bwilkerson commented Aug 29, 2022

IvanDembicki commented Aug 30, 2022

pq commented Aug 3, 2023

pq commented Aug 3, 2023

pq commented Aug 3, 2023

eernstg commented Aug 8, 2023 •

edited

Loading

eernstg commented Aug 9, 2023 •

edited

Loading

pq commented Aug 14, 2023

parlough commented Aug 14, 2023 •

edited

Loading

pq commented Aug 14, 2023

pq commented Aug 14, 2023

parlough commented Aug 14, 2023

bwilkerson commented Aug 14, 2023

pq commented Aug 25, 2023

eernstg commented Aug 28, 2023 •

edited

Loading

srawlins commented Sep 6, 2023 •

edited by pq

Loading

pq commented Sep 6, 2023

srawlins commented Sep 11, 2023 •

edited

Loading

jacob314 commented Sep 12, 2023

srawlins commented Sep 21, 2023

bwilkerson commented Sep 21, 2023

eernstg commented Sep 22, 2023

bwilkerson commented Sep 22, 2023

srawlins commented Sep 26, 2023

lrhn commented Sep 26, 2023

srawlins commented Sep 26, 2023

srawlins commented Oct 30, 2023

pq commented Oct 30, 2023

srawlins commented Oct 30, 2023

pq commented Oct 30, 2023

[Extension type] linter support #58838

[Extension type] linter support #58838

Comments

pq commented Aug 29, 2022 • edited by srawlins Loading

Existing Lints

New Lints

IvanDembicki commented Aug 29, 2022

bwilkerson commented Aug 29, 2022

IvanDembicki commented Aug 30, 2022

pq commented Aug 3, 2023

pq commented Aug 3, 2023

pq commented Aug 3, 2023

eernstg commented Aug 8, 2023 • edited Loading

eernstg commented Aug 9, 2023 • edited Loading

pq commented Aug 14, 2023

parlough commented Aug 14, 2023 • edited Loading

pq commented Aug 14, 2023

pq commented Aug 14, 2023

parlough commented Aug 14, 2023

bwilkerson commented Aug 14, 2023

pq commented Aug 25, 2023

eernstg commented Aug 28, 2023 • edited Loading

srawlins commented Sep 6, 2023 • edited by pq Loading

pq commented Sep 6, 2023

srawlins commented Sep 11, 2023 • edited Loading

jacob314 commented Sep 12, 2023

srawlins commented Sep 21, 2023

bwilkerson commented Sep 21, 2023

eernstg commented Sep 22, 2023

bwilkerson commented Sep 22, 2023

srawlins commented Sep 26, 2023

lrhn commented Sep 26, 2023

srawlins commented Sep 26, 2023

srawlins commented Oct 30, 2023

pq commented Oct 30, 2023

srawlins commented Oct 30, 2023

pq commented Oct 30, 2023

pq commented Aug 29, 2022 •

edited by srawlins

Loading

eernstg commented Aug 8, 2023 •

edited

Loading

eernstg commented Aug 9, 2023 •

edited

Loading

parlough commented Aug 14, 2023 •

edited

Loading

eernstg commented Aug 28, 2023 •

edited

Loading

srawlins commented Sep 6, 2023 •

edited by pq

Loading

srawlins commented Sep 11, 2023 •

edited

Loading