Skip to content

[native_assets_cli] EncodedAsset is not immutable but overrides hashCode #2045

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.

Sign up for GitHub

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

Jump to bottom
Closed
dcharkes opened this issue Feb 28, 2025 · 1 comment · Fixed by #2131
Closed

[native_assets_cli] EncodedAsset is not immutable but overrides hashCode #2045

dcharkes opened this issue Feb 28, 2025 · 1 comment · Fixed by #2131
Labels
package:hooks
Milestone
Native Assets v1.0

Comments

@dcharkes
Copy link
Collaborator

Currently, EncodedAsset takes encoding as an argument, which is a modifiable map.

However, it also overrides hashCode and operator ==.

This is a bug.

We need to either

  1. remove the operator == (and fix the tests that rely on equals) or
  2. copy the map in the constructor
@dcharkes dcharkes added this to the Native Assets v1.0 milestone Feb 28, 2025
@dcharkes dcharkes mentioned this issue Mar 19, 2025
Merged
auto-submit bot pushed a commit that referenced this issue Mar 19, 2025
@dcharkes
[native_assets_cli] Add paths to syntax views (#2107)
543902c 
Bug: #1826

This PR adds the path that was navigated through the JSON to the syntax classes. This makes syntax errors more descriptive: `Unexpected value '123' for 'target_os'. Expected a String.` -> `Unexpected value '123' (int) for 'config.code.target_os'. Expected a String.`.

Because the data asset and code asset extensions don't know the internals of the base protocol extension, the extension points now expose both a `json` and a `jsonPath`. That path is then used to instantiate the syntax view of the extension, so it can give more precise error messages.

* For `CodeConfig` this comes naturally, it's a clean view.
* For `EncodedAsset` this is somewhat weird because it is half-used as value class: #2045

(The next PR will add a full syntax validation pass, at which point we should be able to address #2039.)
@dcharkes
Copy link
Collaborator Author

Ditto for class Metadata.

We have two types of classes in the semantic API:

  1. Pure views on top of a JSON.
    These classes are mirrored by a builder class.
    Objects from these classes cannot be passed from input to output in hooks.
    These classes do not override hashCode and operator ==.
    Examples: CodeConfig, BuildInput.
  2. Data classes that eagerly copy all fields out of the JSON.
    These classes have a public constructor with it's constituent fields.
    Objects from these classes may be passed from input to output in hooks.
    These classes do override hashCode and operator ==.
    Examples: CodeAsset, Architecture, OS, DataAsset.

If data from 1 needs to be passed from an input to an output (or vice versa in the native_assets_builder), then this has to be done by (a) doing it field by field, or (b) taking the json and constructing the dual mirror class with the json.

If we treat classes with maps/lists as 2, these collections need to be deep copied in the constructor (like we would parse out fields), and the accessors need to provide Unmodifiable collections.

Currently both EncodedAsset and Metadata are used as type 2. Maybe they should be moved over to be type 1.

@dcharkes dcharkes mentioned this issue Mar 25, 2025
Merged
@auto-submit auto-submit bot closed this as completed in e2bd08e Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
package:hooks
Projects
Native Assets
Status: Done
Milestone
Native Assets v1.0
Development

Successfully merging a pull request may close this issue.

[native_assets_cli] Prevent EncodedAsset and Metadata hashcode changing dart-lang/native
1 participant
@dcharkes