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

Reject reloads that remove fields from const classes #40442

Closed
rmacnak-google opened this issue Feb 3, 2020 · 3 comments
Closed

Reject reloads that remove fields from const classes #40442

rmacnak-google opened this issue Feb 3, 2020 · 3 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. vm-hot-reload

Comments

@rmacnak-google
Copy link
Contributor

Dart promises that const objects have several properties.

Property 1: Const objects are canonicalized; two const objects with identical fields are identical.
Property 2: Const objects are deeply immutable; they only point to const objects.
Property 3: Const objects have stable hash codes.

Unfortunately, it is not possible to maintain all of these properties after every reload. Consider the program

class C {
  final x, y;
  const C(this.x, this.y);
}
class D {
  final x;
  const D(this.x);
}
const c1 = const C(0, 1);
const c2 = const C(0, 2);
const d1 = const D(c1);
const d2 = const D(c2);

which is reloaded to

class C {
  final x;
  const C(this.x);
}
...

c1 and c2 will lose their y field, and now all their remaining fields are identical. There are a few choices about how to resolve this.

Implementation 1: c1 and c2 remain distinct const objects. Violates property 1, and makes VM asserts about canonicalization fail. Also can break applications that use some mix of identical and ==.
Implementation 2: c1 (or c2) is no longer labeled a const object. Violates property 2 for d1, which can make the compiler perform invalid load folding optimizations, and break similar application-level assumptions.
Implementation 3: c1 has its identity forwarded to c2 (or vice versa). Violates property 3 for c1, and breaks maps or sets containing c1.
or
Limitation 1: Reject any reload that removes fields from a const class.

@rmacnak-google rmacnak-google added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. vm-hot-reload labels Feb 3, 2020
@rmacnak-google
Copy link
Contributor Author

cc @aam @mkustermann

@rmacnak-google rmacnak-google self-assigned this Feb 3, 2020
dart-bot pushed a commit that referenced this issue Feb 5, 2020
Bug: #40442
Change-Id: I84db73dcea909fb2b911ce3def88284d2712d65c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/134383
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
@mraleph
Copy link
Member

mraleph commented Sep 29, 2020

We chose something that is predictable and easy to implement (Limitation 1), but it is unclear if we chose something that is developer friendly (@Hixie's report and user report on Flutter issue tracker indicate that it might not be).

First of all, limitation seems to be applied very early in the reload process - we don't even try to establish if any two constants would collapse when the field is removed. It can very well be that this does not happen all that often.

When I was considering Implementation 3 I also came to realisation that hash codes are going to be affected. There are things we could consider:

  • rehashing builtin maps/sets on reloads if constants got collapsed (would not cover all cases but would cover most);
  • the same applies to normal objects which define hashCode in terms of their fields (rather than using identity hashCode), so maybe the problem is not that bad?

@dcharkes
Copy link
Contributor

dcharkes commented Aug 2, 2021

With our work on #45908, we would have const objects change identityHashCode also on addition of fields.

rehashing builtin maps/sets on reloads

I'll explore the impact of this approach.

(Edit: scratch that, we can also let the canonicalizeHash diverge from identityHashCode after a hotreload, but keep it identical in AOT so that we can avoid saving the hashes if we want to.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. vm-hot-reload
Projects
None yet
Development

No branches or pull requests

3 participants