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

Fixes #2383. Add more constant evaluation tests #2396

Merged
merged 3 commits into from
Dec 4, 2023

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!

I was a little bit worried that the tests in LanguageFeatures/Extension-types/static_analysis_extension_types_A20_t02.dart would be duplicates of language tests that Lasse has added (or is about to add) to $SDK/tests/language/..., but we should in any case check all combinations so I listed a few that I couldn't find currently.

For the function test we'd probably need yet another test library: The current one is fine, but it doesn't test the processing of parameter types.

In particular, we should verify that types like void Function(int) and void Function(ExtInt1) and void Function(ExtInt2) are identical as constants. The reason why this is important is that the canonicalization of function types might use different parts of the implementation to reduce void Function(ExtInt1) to void Function(int) than the parts that we have already exercised with the other tests in this PR.

However, this additional test could certainly be a separate PR, if that's more convenient.

On the other hand, it's probably not important to have additional cases where the function types are nested in other types (that is, tests about Map<ExtFuncInt, ...> and such), because we have tested in the previous test that processing of Map<..., ...> does reach the type arguments. So if the function types work as the outermost part of a type then they probably also work as type arguments of Map. So it's fine that we have List<List<ExtFuncInt>> in a test here, but there's no need to expand on this kind of type in the new test.

But one function type as a subterm of another function type is a case that we need to cover.

I also think it's important that we express those types as function types directly: It is probably going to exercise different locations in the implementation when it handles void Function([ExtInt1]) vs. void Function([int]), compared to the situation where it handles F<ExtInt1> vs. F<int> where typedef F<X> = void Function([X]);.

So we might have a test that succeeds using F, but the test that contains the function types written out in full might still fail.

In summary, for this PR I think we just need to add a few cases that are missing, and then it's a different PR that adds a bunch of extra function type tests.

@eernstg
Copy link
Member

eernstg commented Nov 24, 2023

Oops, I forgot one thing: Assuming the following declaration:

typedef T GenFunction<T>();

GenFunction<T> is an extra case (it will touch some other parts of the implementation than int Function(int)), but it is not a generic function, it's just a different way to produce the type int Function(int). So it would make sense to have some generic function types as well, in that extra test which is probably going into a separate PR.

Edit: ".. a different way to produce the type T Function(T) for a specific T which is the actual type argument to GenFunction (and int Function(int) was just an example)".

@sgrekhov
Copy link
Contributor Author

@eernstg why typedef T GenFunction<T>(); is not a generic function type? Will typedef void GenFunction<T>(T t); be a generic function type?

I'm going to add all missing parts (including parametrized function types) to this PR if ypu don't mind

@eernstg
Copy link
Member

eernstg commented Nov 24, 2023

why typedef T GenFunction<T>(); is not a generic function type?

GenFunction is a compile-time function that maps a type argument T into a non-generic function type. For instance, GenFunction<int> is the same thing as int Function().

A generic function type is the type of a function that accepts one or more actual type arguments at each invocation. For instance, X Function<X>(X) is a generic function type, and a function f with that type can be called as f<T>(t) where T is some term that denotes a type (which may or may not be known at compile time, so the passing of actual type arguments to f must be a run-time phenomenon), and t is an expression of type T.

The old-style type alias declaration cannot declare a generic function type, it can only declare a family of non-generic function types where each reference to the type alias must provide actual type arguments (or they will be selected using instantiation to bound).

In order to declare a generic function type using a type alias, use the new syntax: typedef F = X Function<X>(X);. For comparison, typedef G<X> = X Function(X); has the same meaning as typedef X G<X>(X);, and both of these declare a family of non-generic function types.

@eernstg
Copy link
Member

eernstg commented Nov 24, 2023

add .. to this PR

OK!

@sgrekhov
Copy link
Contributor Author

@eernstg is Ok that we have no error for ExtFuncGen2 below?

typedef T GenFunction1<T>(T t);
typedef GenFunction2<T> = T Function<T>(T t);

extension type const ExtFuncGen1<T>(GenFunction1<T> _) {} //  Error: An extension type parameter can't be used non-covariantly in its representation type.
extension type const ExtFuncGen2<T>(GenFunction2<T> _) {} // No error

@eernstg
Copy link
Member

eernstg commented Nov 27, 2023

Yes, that's OK: The type parameter T of GenFunction2 is never used, because every occurrence of T on the right hand side of = is a reference to, or a declaration of, a type parameter of a generic function type. So it might as well have been defined as follows, which would arguably have been more readable:

typedef GenFunction2<T> = X Function<X>(X t);

@sgrekhov
Copy link
Contributor Author

Updated. Please take another look

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.

Maybe I forgot where it is, but I think we don't cover canonicalization of function types where extension types occur as subterms. So we can do it here or in a separate PR, whatever fits best for you.

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Dec 4, 2023

Updated. Please take another look

@sgrekhov sgrekhov requested a review from eernstg December 4, 2023 17:40
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 95141a4 into dart-lang:master Dec 4, 2023
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 11, 2023
2023-12-07 sgrekhov22@gmail.com Fixes dart-lang/co19#2413. Add missing expected compile-time errors for CFE (dart-lang/co19#2418)
2023-12-07 sgrekhov22@gmail.com dart-lang/co19#2119. Run dart formatter on LibTest/async tests (dart-lang/co19#2417)
2023-12-06 sgrekhov22@gmail.com dart-lang/co19#2398. Make asyncStart/End() safe in LibTest/async (dart-lang/co19#2416)
2023-12-06 sgrekhov22@gmail.com Fixes dart-lang/co19#2355. Add more typed_date.setRange() tests (dart-lang/co19#2412)
2023-12-06 sgrekhov22@gmail.com dart-lang/co19#2404. Small code-style improvements and description update (dart-lang/co19#2414)
2023-12-04 sgrekhov22@gmail.com dart-lang/co19#2004. Add deferred libraries tests according to the changed spec (dart-lang/co19#2411)
2023-12-04 sgrekhov22@gmail.com Fixes dart-lang/co19#2383. Add more constant evaluation tests (dart-lang/co19#2396)

Change-Id: I15e0d681538fa0f2a311f74d1930fad7270b81a0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/340561
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
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.

None yet

2 participants