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

#2976. Add semantics tests #2998

Merged
merged 11 commits into from
Nov 28, 2024
Merged

#2976. Add semantics tests #2998

merged 11 commits into from
Nov 28, 2024

Conversation

sgrekhov
Copy link
Contributor

No description provided.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with a couple of comments!

The feature specification talks about "like an accessible type alias" which is a somewhat Byzantine way to say that we can ignore type arguments as needed (namely, when we're invoking a static member of the context type).

This means that we basically can't test whether it is actually "like an accessible type alias", because there's no way we could do anything other than ignoring any actual type arguments in cases like .myStaticMethod() with context type C<String>.

However, it gets worse: Constructor invocations will ignore the actual type arguments in the context type, and use normal inference to find them. Often, this means to "re-find" the actual type arguments because we will frequently infer exactly the same type arguments that we just erased. However, it actually matters in some cases, especially with non-trivial selector chains.

So I'm suggesting that the @description is adjusted to say that these tests are just testing that when we're computing a static namespace from a given context type we will also expand all type aliases.

///
/// @description Checks that expressions of the form `.id`, `.id(args)` and
/// `.id<typeArgs>(args)` are treated as if they are prefixed by a fresh
/// identifier `X` which denotes an accessible type alias.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really hard to test! ;-)

The point could be that a type alias can be non-generic and still denote a parameterized type (typedef X = SomeType<With, Args>;), and this allows for the constructor invocations to have the actual type arguments, and it also allows for a static method invocation to ignore the same type arguments (because that's how we treat type aliases when used to access static members).

Apart from this, there would be no distinction between this description and a shorter one that simply says "the semantics of .id is T.id, where T is the greatest closure of the context type.

However, that works for invocations of static methods, but it is not the actual semantics of constructor invocations: For them we strip off the actual type arguments (so the greatest closure operation is ignored ... for both static member invocations and constructor invocations!), and then type inference is performed as if no actual type arguments were passed.

This means that it isn't hugely meaningful to test "as if prefixed by a type alias" at all.

What we need to test is that it works to call a static method even in the case where the context type has actual type arguments (where those actual type arguments must be ignored).

Next, we should test that it works to invoke a constructor in a case where the context type has actual type arguments (e.g., List<num> as context type should make .filled(10, 10) mean List<num>.filled(10, 10), and the type argument should not be int, as it would be if the context type were _).

Finally, it should work to invoke a constructor with a context type schema (say, List<Iterable<_>>), and we should then see that the actual arguments to the constructor invocation plays a role:

Object foo<X>(Map<int, X> map) => map;
var map = foo(.fromIterables([1], ['Hello']));

In this case we'd get int from the context type and String from the actual argument ['Hello'], and the result would then be a Map<int, String>.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really hard to test! ;-)

And the spec also doesn't use the "like a type alias" phrasing any more. Since yesterday.

What's important to test is that it:

  • Accesses the correct static member.
  • Does so, even if the name of that type is shadowed, or not even in scope.

so .filled with context type List<int> denotes (the constructor) List.filled. Even if List is not in scope.

The current spec also doesn't use "the greatest closure of the context type" any more. An occurrence of _ doesn't matter, it's either in a position where it's irrelevant, or the scheme is _ in which case it doesn't denote a static namespace.

To find the static namespace, it just looks at the context type scheme, and if it's of the form S? or FutureOr<S>, it recurses on the S, and otherwise it only recognizes types of the form C/C<typeArgs> where C is a type introduced by a class, mixin, enum or extension type declaration.

(It's written declaratively, "a type scheme denotes a static namespace N if and only if either the type scheme is a type introduced by a type declaration and N is the static namespace of that declaration, or the type scheme is either S? or FutureOr<S> and S denotes the static namespace N.".)

So if the context type scheme is a non-generic type or instantiated generic type scheme whose type is introduced by the declaration D, then .id denotes the static member named id or constructor with base name id in D.
And if it's a constructor, then it's a raw reference to that constructor, where type arguments to the class can be inferred from arguments or actual context type (not shorthand context, in the cases where those differ.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Does so, even if the name of that type is shadowed, or not even in scope.
    so .filled with context type List<int> denotes (the constructor) List.filled. Even if List is not in scope.

How can we have a context type List<int> if List is not in scope or shadowed? It's just a compile-time error.

class C {
  int List() => 42;
  
  test() {
    List<int> l = List.filled(1, 1);
//  ^^^^  Error: List isn't a type
  }
}
class List<T> {}

main() {
  List<int> l = List<int>.filled(1, 1);
  //                      ^^^^^^ Error: Couldn't find constructor 'List.filled'.
}

Please, clarify.

Copy link
Member

@eernstg eernstg Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we have a context type List<int> if List is not in scope or shadowed?

You can declare a function like void foo<X>(List<X> xs) {} in another library 'lib.dart', and then call foo from a library that imports 'lib.dart' and imports 'dart:core' with a prefix such that List isn't in scope without the prefix. foo(.filled(1, 1)) should then work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eernstg eernstg merged commit 67eddf2 into dart-lang:master Nov 28, 2024
2 checks passed
@sgrekhov
Copy link
Contributor Author

Thank you for the detailed review and explanation! I've updated tests descriptions to say that we are testing that processing of a context type includes type aliases (is there a better place in the spec to test it?). In the main() function the only checks for the type aliases left (there are and there will be plenty of checks for non-type aliases). In the tests

 if (o is C) {
   o = .id<int>;
...
 }

I'm testing not type aliases only because there are no such tests yet.

Tests that check that type arguments should be ignored will be added in a separate PR as a type inference tests.

@eernstg
Copy link
Member

eernstg commented Nov 28, 2024

Sounds good, thanks!

One could say that we're testing that it is not only an alias expansion step (which could transform MyAlias.staticMethod() into MyClass<int>.staticMethod()) but it is also a step that eliminates any actual type arguments that would have been an error (turning MyClass<int>.staticMethod() into MyClass.staticMethod()). This scenario is indeed covered by CInt, MInt, etc.

Invocations of constructors are not covered in this PR (IIRC), but they would have a corresponding behavior to test: The actual type arguments in the context type should be stripped off and then re-computed using regular type inference. I created dart-lang/language#4183 to clarify that this step includes a full alias expansion.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 1, 2024
2024-11-28 [email protected] dart-lang/co19#2976. Update assertion texts according to the new spec (dart-lang/co19#3000)
2024-11-28 [email protected] dart-lang/co19#2119. Add issue number to other_declarations_A04_t07.dart (dart-lang/co19#3001)
2024-11-28 [email protected] dart-lang/co19#2976. Add semantics tests (dart-lang/co19#2998)
2024-11-27 [email protected] Fixes dart-lang/co19#2996. Update CFE expected errors locations (dart-lang/co19#2999)
2024-11-25 [email protected] dart-lang/co19#2559. Add extension types and enums to augmenting constructors test. Part 2. (dart-lang/co19#2972)
2024-11-25 [email protected] dart-lang/co19#2956. Add more type `void` tests (dart-lang/co19#2997)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: I858dbf6908fe32f38a401ce4ebfe120e064de4aa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398160
Reviewed-by: Alexander Thomas <[email protected]>
Commit-Queue: Alexander Thomas <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants