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

Analyzer rejects implementing alias that expands to type. #54551

Open
lrhn opened this issue Jan 9, 2024 · 7 comments
Open

Analyzer rejects implementing alias that expands to type. #54551

lrhn opened this issue Jan 9, 2024 · 7 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@lrhn
Copy link
Member

lrhn commented Jan 9, 2024

typedef A<T> = T;
extension type EIAliasInt(int _) implements A<int> {}

Both CFE and analyzer rejects this extension type, saying that you can't implement an alias that expands to a type variable. (Checked in DartPad, master branch.))

It should be accepted.

The behavior seems (based on the error message) like an incorrect interpretation of the rule:

A compile-time error occurs if an extension type has a non-extension superinterface whose transitive alias expansion is a type variable, a deferred type, any top type (including dynamic and void), the type Null, any function type, the type Function, any record type, the type Record, or any type of the form T? or FutureOr<T> for any type T.

The "transitive alias expansion is a type variable" should be applied to the actual superinterface type term, A<int>, not to the alias declaration A<T>.

The places where you can't use an alias like A are places where the alias is not used as a type, but as a scope, and we want to know what the scope is statically.

The analyzer says:

error
line 2 • A type alias that expands to a type parameter can't be implemented. (view docs)
Try specifying a class or mixin, or removing the list.

The linked diagnostic explanation does suggest that the behavior is intentional, and lists class C implements A<Object> {} as something that should err.

@eernstg Am I missing something here? Did we disallow implements A<Object> even if it's supposedly equivalent to implements Object?

@lrhn lrhn added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) feature-extension-types Implementation of the extension type feature labels Jan 9, 2024
@eernstg
Copy link
Member

eernstg commented Jan 9, 2024

We do actually have this error:

A compile-time error occurs if D expands to a type variable, and T uses D as a class.

where D is a type alias declaration named F, and T is a type of the form F<T1 .. Tk>.

This does kick in when A<Object> is used as a superinterface, e.g., in the implements clause of a class. I agree that this is overkill: We wanted to prevent class C<X> implements X {}, but this also prevents something that just means class C implements Object {} and similar benign things.

The rule would have to be somewhat more involved in order to handle cases like this:

typedef F<X> = X;
typedef F2<X, Y> = X;

class C<X> implements F2<F<F<F<X>>>, F<F<F<F<Object>>>>> {} // Error.
class C<X> implements F2<F<F<F<Object>>>, F<F<F<F<X>>>>> {} // OK.

So we're being a little bit lazy, but the question is whether we want to handle the extra machinery which is needed (whatever that is) in order to allow these "dangerous" type aliases to be used as superinterfaces and as the class which is being instantiated in instance creations, etc.

Do we prevent any useful idioms by keeping it simple?

@eernstg
Copy link
Member

eernstg commented Jan 9, 2024

We might be able to simplify the notion of 'being used as a class', because we already have the other rule that you mentioned:

A compile-time error occurs if an extension type has a non-extension superinterface whose transitive alias expansion is a type variable ..

However, the rules that we have for classes, mixins, and enums are a bit simpler, for example:

It is a compile-time error if an element in the type list of the implements clause of a class is
... a type alias that does not denote a class

So we might want to talk about the transitive alias expansion in all these cases (not just extension types) and then take superinterfaces out of the definition of 'use as a class'.

A simpler fix would be to take the 'transitive alias expansion' out of the extension type rule, and keep the (still rather lazy) rule about superinterfaces in 'use as a class'.

It does seem intuitively right that we include at least superclasses in the definition of 'use as a class', but implements and mixin-on clauses may be less well justified.

@lrhn
Copy link
Member Author

lrhn commented Jan 10, 2024

My personal preference is that we should never talk about a type alias, only about is expansion, except where we define said expansion, and where we say which static members you can access through the type alias (where we are really aliasing a namespace as well, not a type).

There might be reasonable reasons for what we're doing here, even if it's only making our own job easier. Probably only that.

I like to think of the static part of compilation as being somewhat staged, and being able to rely on the prior stages not having compile-time errors.
That means checking that, fx, a class declaration parses before introducing any types for it, and checking that the type hierarchy is well-defined (super-class/mixin/interfaces exist and are valid declarations, and it have no cycles) before introducing subtyping. Before that, the subtyping relation simply doesn't exist, because it requires all types to exist first, and then it requires that the types can be ordered.

If we require type aliases to be expanded when we check whether the superinterface exists, we migth so before we have introduced subtyping, which means we cannot check any bounds at that point. We're not doing real type alias expansion, because that requires primitives that do not exist yet. All we can do at this point is resolve identifiers to declarations.
That should be enough. We can detect type-alias cycles using just that, and we can introduce all the types for all type-introducing-declarations, even if we can't say which ones of them are well-bounded yet (maybe not even which ones have the correct number of type arguments).

We should be able to do symbolic expansion of type aliases, without checking any bounds, to figure out that A<Object> refers to the declaration Object, A<List<int>> refers to the declaration List (never-mind the rest of the type arguments for now), and A<T> refers to the type variable T, and is therefore not a valid implements clause entry.
We'll only need to find the first head-term that is not an alias, the rest will be dealt with in later phases. That is "does this type alias instantiation refer to a declaration, and if so, which one?", and nothing more.
(We probably also need to define "instantiate to bounds" at this time, otherwise we can't expand type raw alias references. I believe that's possible too.)

But that does require defining a separate alias expansion algorithm, just for this.
(Not sure disallowing type aliases which expand to one of their own type variables helps, though, because we still do need to expand the rest to see which declaration they point to. But we might be able to reuse the test for "which declaration does this alias" that we also use for accessing static members through an alias.)

All in all, I think this restriction is unnecessary, but I can't say whether it does make something easier for implemnters.

It also feels inconsistent:

typedef A<T> = T;
typedef AC = A<C>;
typedef AD = A<D>;

class C {}
class C0 implements A<C> {} // Error, A<C> is not a valid superinterface.
class C1 implements AC {} // No problem AC, alias for A<C>, is perfectly fine.
class D implements AD {} // Whoa, cyclic reference!

It's not the type that is the problem. It's apparently also not the ability to expand type aliases that is a problem, because we do that anyway for D.

So, the rule doesn't seem to make everything magically easier, it's not needed to disallow implementing a type variable, just expand the alias and see that the result is a type variable, then complain like we would for a directly written type variable.

@eernstg
Copy link
Member

eernstg commented Jan 10, 2024

Note that there is an easy workaround, we can just introduce one extra layer of type aliasing:

// Example 1

typedef A<T> = T; // Cannot be used as a class.
typedef Aint = A<int>; // Add one layer, _can_ be used as a class.

extension type EIAliasInt(int _) implements Aint {} // OK.

// Example 2

typedef F<X> = X;
typedef F2<X, Y> = X;

typedef F2a<X> = F2<F<F<F<X>>>, F<F<F<F<Object>>>>>; // Cannot be used as a class.
typedef F2b<X> = F2<F<F<F<Object>>>, F<F<F<F<X>>>>>; // Can be used as a class.

class C<X> implements F2a<X> {} // Error, `F2a` cannot be used as a class.
class C2<X> implements F2b<X> {} // OK.

// Example 3 -- essentially already uses the extra layer of type aliases.

typedef A<T> = T; // Cannot be used as a class.
typedef AC = A<C>; // Alias for `C`.
typedef AD = A<D>; // Alias for `D`.

class C {}
class C0 implements A<C> {} // Error, just can't use `A` directly.
class C1 implements AC {} // OK.
class D implements AD {} // Error, cannot implement itself.

This would serve as an argument in favor of keeping the rules as they are today: No expressive power is lost by coloring each type alias as "can be used as a class" or "cannot be used as a class", and then proceeding to report an error for every occurrence of a "cannot" type alias which is used as a class.

Note that "being used as a class" applies to other constructs as well, not just superinterfaces: It is used with static member lookups and it is used with instance creation expressions. I do think that the notion of "being used as a class" is reasonably intuitive, which might be an argument in favor of using that concept (say, in error messages. As it is today.).

In summary, you're right that the currently specified approach is more restrictive than it must be. I'm just not convinced that this amount of overkill creates any real problems.

@lrhn
Copy link
Member Author

lrhn commented Jan 10, 2024

This would serve as an argument in favor of keeping the rules as they are today

Arguably, it's also an argument in favor of removing the unnecessary restriction, since it doesn't actually restrict anything.
We just don't have to do so eagerly, since there is a workaround.

@eernstg
Copy link
Member

eernstg commented Jan 10, 2024

Arguably, it's also an argument in favor of removing the unnecessary restriction, since it doesn't actually restrict anything.

I don't understand? The restriction is that no parameterized type F<T1 .. Tk> where F is a type alias that expands to a type variable can be used as a superinterface. I do think that restricts something (and even more than strictly necessary).

The rule about extension types that talks about the transitive expansion of type aliases is special (we don't have the same kind of rule for classes, mixins, etc.), so we'd need to go over all relevant constructs and ensure that they get the protection they need. This is true for instance creations and redirecting constructors as well.

@lrhn
Copy link
Member Author

lrhn commented Jan 10, 2024

I do think that restricts something

It's a syntactic restriction, which prevents you from writing a thing in a specific way, without actually preventing you from achieving the same semantics in a different way.
It doesn't prevent you from doing anything, it just restricts how you can write it.

That can be fine in some cases, where a particular way of working things can be confusing, but this is not one of those cases. So it's just an unnecessary restriction, with no real benefit.
(That's assuming we still prevent you from actually implementing a type variable, if an aliased type expands to it.)

@leafpetersen leafpetersen removed the feature-extension-types Implementation of the extension type feature label Jan 11, 2024
@pq pq added the P2 A bug or feature request we're likely to work on label Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants