#2976. Add more tests for records, factories and parenthesized expressions #3047
Conversation
eernstg
left a comment
eernstg
left a comment
There was a problem hiding this comment.
Looks good!
I just don't understand the connection between the assertion and the test in the first one.
| /// | ||
| /// @description Checks that expressions of the form | ||
| /// `const '(' '.' id(arguments) ',' ')'` and | ||
| /// `const '(' '.' new(arguments) ',' ')'` are constant expressions. |
There was a problem hiding this comment.
I can't really see the connection to the assertion: The assertion is about an expression e of the form <staticMemberShorthandHead> <selector>* or '.' <identifierOrNew>? or 'const' or 'const' '.' or 'const' '.' <identifierOrNew> or 'const' '.' <identifierOrNew> <arguments> (or a bunch of other forms which are prefxies of 'const' '.' <identifierOrNew> <arguments> that I haven't already mentioned) ...
(I'm not 100% convinced we want to allow for all those forms, but anyway)
then consider the case where e also has the form 'const' '.' <identifierOrNew> <arguments>. In this case e is a constant expression, except that it is an error if it doesn't denote a constant constructor, or any argument is non-constant.
There's no reference to record literals anywhere, so where do they come from?
I think the natural thing to test would be that const .name(14) is a constant expression with context type C when C.name is a constant constructor that accepts this actual argument list.
Testing constant records is at least one step further away and doesn't actually test the use of const .name(14). It actually tests the use of .name(14) as a constant expression because .name(14) occurs in a constant context when it occurs as const (.name(14),).
So the test is OK in its own right, if the purpose is to test that constant record literals can contain static access shorthands denoting a constructor invocation, but it doesn't quite match the assertion.
There was a problem hiding this comment.
If we don't already have a test where a constant record literal contains a static access shorthand then we could just take a copy of this one and change the assertion, and then adjust this one to work on const .name(14) rather than const (.name(14),).
There was a problem hiding this comment.
The idea here was to test parenthesized expressions. To check that (.id) is treated as .id. But in case of constants const (...) is always parsed as a constant record. I think it makes sense to check that const (.id,) is still treated as a record and const (.id) is not a parenthesized constant but a syntax error. Moved to grammar tests with the apropriate assertion (can't find the better one). PTAL.
| /// `const '(' '.' id(arguments) ',' ')'` and | ||
| /// `const '(' '.' new(arguments) ',' ')'` are constant expressions. | ||
| /// `const '(' '.' new(arguments) ',' ')'` are records containing constant | ||
| /// shorthand expressions. |
| /// | ||
| /// @description Checks that expressions of the form | ||
| /// `const '(' '.' id(arguments) ')'` and | ||
| /// `const '(' '.' new(arguments) ')'` are still compile-time errors. |
| /// | ||
| /// @description Checks that expressions of the form | ||
| /// `const '(' '.' id(arguments) ',' ')'` and | ||
| /// `const '(' '.' new(arguments) ',' ')'` are constant expressions. |
2025-01-17 sgrekhov22@gmail.com Fixes dart-lang/co19#3044. Update expected errors positions for CFE (dart-lang/co19#3046) 2025-01-15 sgrekhov22@gmail.com dart-lang/co19#2976. Add more tests for records, factories and parenthesized expressions (dart-lang/co19#3047) 2025-01-15 sgrekhov22@gmail.com dart-lang/co19#2119. Actualize nnbd/static_errors_A12* description (dart-lang/co19#3048) 2025-01-14 sgrekhov22@gmail.com Fixes dart-lang/co19#3029. Add more tests for constants (dart-lang/co19#3045) 2025-01-14 sgrekhov22@gmail.com dart-lang/co19#2976. Add patterns tests (dart-lang/co19#3031) 2025-01-13 sgrekhov22@gmail.com dart-lang/co19#2976. Add more operators tests (dart-lang/co19#3043) 2025-01-13 sgrekhov22@gmail.com dart-lang/co19#3030. Add tests constants tests for String.length (dart-lang/co19#3041) 2025-01-13 sgrekhov22@gmail.com Fixes dart-lang/co19#3038. Read link to finalized object to avoid early finalization (dart-lang/co19#3042) 2025-01-10 sgrekhov22@gmail.com dart-lang/co19#2976. Add non-ambiguity tests (dart-lang/co19#3034) 2025-01-10 sgrekhov22@gmail.com dart-lang/co19#2976. Add more grammar tests (dart-lang/co19#3035) Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try Change-Id: I9feb50a33b33e85dc017d0d47829502d4f631ac7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405001 Reviewed-by: Alexander Thomas <athom@google.com> Reviewed-by: Erik Ernst <eernst@google.com> Commit-Queue: Erik Ernst <eernst@google.com> Commit-Queue: Alexander Thomas <athom@google.com>
2 participants
No description provided.