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

[cfe/analyzer] No error when extension type Type is duplicated with its underlying type's in constant sets and maps #53918

Closed
parlough opened this issue Oct 31, 2023 · 7 comments
Assignees
Labels
analyzer-constants area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. feature-extension-types Implementation of the extension type feature P2 A bug or feature request we're likely to work on

Comments

@parlough
Copy link
Member

parlough commented Oct 31, 2023

Currently the following passes analysis and compiles, but I think it shouldn't since at runtime E and int are equal and const sets/maps don't allow elements/keys that are equal. I haven't read the full extension type spec though, so I might be missing something :)

Currently compiling and passing analysis:

extension type E(int value) {}

const testSet = {int, E};
const testMap = {int: 1, E: 2};

Expected:

const testSet = {int, E}; // error: equal_elements_in_const_set
const testMap = {int: 1, E: 2}; // error: equal_keys_in_const_map

Dart SDK version: 3.3.0-edge.8b3d93234d1df69c3db5428fa02bb6b39c671ed8

@parlough parlough changed the title [cfe/analyzer] No error when extension type is duplicated with its underlying type in constant sets and maps [cfe/analyzer] No error when extension type Type is duplicated with its underlying type's in constant sets and maps Oct 31, 2023
@bwilkerson
Copy link
Member

@eernstg Is the instance of Type represented by int the same as the instance of Type represented by E in the example above?

@eernstg
Copy link
Member

eernstg commented Oct 31, 2023

Yes, the run-time representation of an extension type is the 'extension type erasure' (sometimes known as the ultimate representation type), and in this case it's int, so identical(E, int) is true and the errors are fine.

@eernstg
Copy link
Member

eernstg commented Oct 31, 2023

(It's surprising that those errors aren't reported: I thought such things as "duplicate element in constant set" were checked based on the a static-analysis stand-in for the object which is the result of a constant evaluation, and that would be the Type for int in both cases, so there wouldn't have to be any new actions in order to determine that it's a duplication.)

@bwilkerson
Copy link
Member

@scheglov @kallentu

My guess is that we either haven't implemented constant evaluation for extension types or we've failed to reduce the type to the extension type erasure before creating the representation of DartObject for that type.

@lrhn
Copy link
Member

lrhn commented Oct 31, 2023

Constant evaluation with extension types is somewhat special in that it should use the runtime semantics, which have erased extension types, but every result can still have a static type which is an extension type.
Pure runtime evaluation can completely forget extension types, but constant evaluation has to move back and forth between the static type system and the runtime type system.

That is: We need tests. Lots of tests.

@mit-mit mit-mit added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Nov 1, 2023
@pq pq added analyzer-constants P2 A bug or feature request we're likely to work on labels Nov 1, 2023
@scheglov scheglov self-assigned this Nov 14, 2023
@lrhn
Copy link
Member

lrhn commented Nov 14, 2023

I'm working on "tests, lots of tests", and while they're not complete yet, and probably not all correct yet, I am seeing similar issues when directly comparing constant type expressions for identity.

https://dart-review.googlesource.com/c/sdk/+/335560/2/tests/language/extension_type/constant_computation_test.dart

@scheglov
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/336501

copybara-service bot pushed a commit that referenced this issue Nov 15, 2023
Bug: #53918
Change-Id: I941332f30bdc681dd10c8471e1710f2e9dde4c68
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336501
Reviewed-by: Kallen Tu <kallentu@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@lrhn lrhn added the feature-extension-types Implementation of the extension type feature label Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-constants area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. feature-extension-types Implementation of the extension type feature P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

7 participants