-
Notifications
You must be signed in to change notification settings - Fork 28
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 type inference tests. Part 2 #3005
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, couple of comments.
One thing needs to be reconsidered: It's the context type schema of .id.a.b.c
which is used to compute the static namespace where id
is looked up, not the context type schema of .id
.
@@ -55,4 +55,9 @@ main() { | |||
// [analyzer] unspecified | |||
// [cfe] unspecified | |||
}.toList(); | |||
|
|||
dynamic v = .parse("42"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I'm not sure dynamic
as a context type schema is equivalent to _
. But we should have the error anyway, and it's a delicate question, so let's have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal states that a context type schema in this case is _
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... I can't see that this has been specified anywhere, but the choice of context type schema in every given situation is highly underspecified anyway.
One source of confusion is that the implemented type inference algorithm chooses dynamic
in a number of cases where the specification in inference.md says that the inferred result is the greatest closure. I created dart-lang/language#4197 to shed some light on that.
If we assume that this is fixed then I'd expect the following behavior:
X getContextType<X>(Object? o) {
print(X);
return o as X;
}
void main() {
getContextType(1)..arglebargle; // Compile-time error.
dynamic d = getContextType(1)..arglebargle; // No compile-time error.
}
This should illustrate that the context type schema _
and the context type dynamic
is not the same thing. So we still need some clarifications, but I'd maintain that it isn't safe to assume that they are the same thing.
LanguageFeatures/Static-access-shorthand/type_inference_A07_t01.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A07_t02.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A07_t02.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A07_t02.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A08_t01.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A08_t02.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I misunderstood the spec at first. Now updated, PTAL
@@ -55,4 +55,9 @@ main() { | |||
// [analyzer] unspecified | |||
// [cfe] unspecified | |||
}.toList(); | |||
|
|||
dynamic v = .parse("42"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal states that a context type schema in this case is _
.
LanguageFeatures/Static-access-shorthand/type_inference_A07_t01.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A07_t02.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A07_t02.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A07_t02.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A08_t01.dart
Outdated
Show resolved
Hide resolved
LanguageFeatures/Static-access-shorthand/type_inference_A08_t02.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
2024-12-06 [email protected] Fixes dart-lang/co19#3008. Rewrite type_inference_A01_t06.dart to get the context type _ (dart-lang/co19#3009) 2024-12-06 [email protected] Fixes dart-lang/co19#3006. Fix a typo in type_inference_A01_t06.dart (dart-lang/co19#3007) 2024-12-06 [email protected] dart-lang/co19#2976. Add type inference tests. Part 2 (dart-lang/co19#3005) 2024-12-05 [email protected] dart-lang/co19#2976. Add more type inference tests (dart-lang/co19#3004) 2024-12-04 [email protected] dart-lang/co19#2976. Add grammar, semantics and type inference test for constants (dart-lang/co19#3003) 2024-11-29 [email protected] dart-lang/co19#2976. Add more grammar, semantics and type inference test (dart-lang/co19#3002) Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try Change-Id: Ie10a17900bc7d61d4f9288d3e3d2f359eca4f1d9 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/399400 Commit-Queue: Alexander Thomas <[email protected]> Reviewed-by: Erik Ernst <[email protected]> Reviewed-by: Alexander Thomas <[email protected]>
No description provided.