Groups keyboard shortcuts have been updated
Dismiss
See shortcuts
[L] Change in dart/sdk[main]: Reland "[ddc] Overhauling tearoff equality and identity."
0 views
Skip to first unread message
Mark Zhou (Gerrit)
Mar 14, 2025, 4:24:14 PMMar 14
to Nate Biggs, Nicholas Shahan, Commit Queue, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Attention needed from Nate Biggs and Nicholas Shahan
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Nate Biggs
- Nicholas Shahan
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Mar 14, 2025, 6:48:00 PMMar 14
to Mark Zhou, Nate Biggs, Commit Queue, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Attention needed from Mark Zhou and Nate Biggs
Nicholas Shahan added 3 comments![]( "Open in Gerrit")
File pkg/dev_compiler/lib/src/kernel/compiler.dart
Line 1448, Patchset 8 (Latest):
var enclosingLibrary = _emitLibraryName(getLibrary(m));Nicholas Shahan . unresolved
I'd prefer `_currentLibrary!` here. If this is not the same thing, I unsure about the assignment below. We would be installing this into a different library?
File pkg/dev_compiler/lib/src/kernel/compiler_new.dart
Line 5641, Patchset 8 (Latest):
// Create a property accessor for cross-library mixins.Nicholas Shahan . unresolved
Why do the accessors look different depending on what library they are from? I'd expect `LibraryName.MixinApplicationClassName` in the compile code for both cases even when in the current library to make sure we are always going through the library indirection.
Related details
Attention is currently required from:
- Mark Zhou
- Nate Biggs
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mar 14, 2025, 7:52:01 PMMar 14
to Nate Biggs, Nicholas Shahan, Commit Queue, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Attention needed from Nate Biggs and Nicholas Shahan
Mark Zhou added 3 comments![]( "Open in Gerrit")
File pkg/dev_compiler/lib/src/kernel/compiler.dart
Line 1448, Patchset 8:
var enclosingLibrary = _emitLibraryName(getLibrary(m));Nicholas Shahan . resolved
I'd prefer `_currentLibrary!` here. If this is not the same thing, I unsure about the assignment below. We would be installing this into a different library?
Mark Zhou
This should be fine in `emitClass`. We only see cross-module mixins in the super getters. Updated.
File pkg/dev_compiler/lib/src/kernel/compiler_new.dart
Line 5641, Patchset 8:
// Create a property accessor for cross-library mixins.Nicholas Shahan . resolved
Why do the accessors look different depending on what library they are from? I'd expect `LibraryName.MixinApplicationClassName` in the compile code for both cases even when in the current library to make sure we are always going through the library indirection.
Mark Zhou
True. I added this since it was 'extraneous' if in the same library. But going through the indirection is prob actually correct. Updated.
Related details
Attention is currently required from:
- Nate Biggs
- Nicholas Shahan
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Mar 17, 2025, 12:24:22 PMMar 17
to Mark Zhou, Nicholas Shahan, Commit Queue, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Attention needed from Mark Zhou and Nicholas Shahan
Nate Biggs voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Mark Zhou
- Nicholas Shahan
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Mar 17, 2025, 12:31:57 PMMar 17
to Mark Zhou, Nate Biggs, Commit Queue, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Attention needed from Mark Zhou
Nicholas Shahan voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Attention is currently required from:
- Mark Zhou
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Zhou (Gerrit)
Mar 17, 2025, 12:47:55 PMMar 17
to Nicholas Shahan, Nate Biggs, Commit Queue, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Mark Zhou voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Mar 17, 2025, 1:34:53 PMMar 17
to Mark Zhou, Nicholas Shahan, Nate Biggs, Srujan Gaddam, dart-dc-te...@google.com, rev...@dartlang.org
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Reland "[ddc] Overhauling tearoff equality and identity."
This new update adds fixes + tests for cross-module super mixins. Super getters can refer to mixins compiled in other modules, so we need to expose all mixin names to make them accessible from their enclosing library.
This is a reland of commit e4c4d0f839ce4998a46ac3997aea2668d3edd48f
Original change's description:
> [ddc] Overhauling tearoff equality and identity.
>
> Hot reload requires DDC to update how its tearoffs are represented. Tearoffs obey the following conventions:
> * Instance tearoffs are never identical
> * Tearoffs with the same object target and name have the same hash code (even if they resolve to different functions across hot reloads)
> * Two separate tearoffs of the same member are equal
>
> To support this, tearoff equality must not depend on the bound object and method but a composite of the bound object, torn off member name, and the exact class/object from which the member was torn off.
>
> Notable changes:
> * Methods' immediately bound targets are emitted with member signatures. This is required to determine the bound targets for instance and dynamic tearoffs. Bound targets are identified by `libraryUri:class` strings.
> * `applyMixin` passes in a 'true' bound target. This is because mixin applications' members are considered children of their 'on' class (not the mixed in class) wrt equality/hashCode.
> * `bind` is modified to pass in its 'true' bound object to support mixins' super getters.
> * `tearoff` and `staticTearoff` are modified to accept a bound target string (only required for static tearoffs, as they are bound at tearoff-creation-time).
> * Static tearoffs avoid using their bound object for hashcode and equality, as these libraries may be wrapped in proxy objects.
> * Tearoff equality and hashCode are updated to consider bound object, bound name, and its bound method's immediate target.
> * 'noSuchMethod' and 'toString' methods are always accessed through their extension property during signature lookups.
>
>
> Change-Id: Ica5501b6860c605db50aa945bafb6802a7317511
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/406723
> Reviewed-by: Nate Biggs <nate...@google.com>
> Reviewed-by: Nicholas Shahan <nsh...@google.com>
> Commit-Queue: Mark Zhou <mark...@google.com>
Change-Id: I645992030f9106c158426412387ddecafc78e3f4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/415102
Reviewed-by: Nate Biggs <nate...@google.com>
Commit-Queue: Mark Zhou <mark...@google.com>
Reviewed-by: Nicholas Shahan <nsh...@google.com>
Files:
- M pkg/dev_compiler/lib/src/kernel/compiler.dart
- M pkg/dev_compiler/lib/src/kernel/compiler_new.dart
- M pkg/dev_compiler/test/expression_compiler/expression_compiler_worker_shared.dart
- M pkg/dev_compiler/test/expression_compiler/runtime_debugger_api_test.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/classes.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/debugger.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/runtime.dart
- M sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/utils.dart
- M sdk/lib/_internal/js_dev_runtime/private/interceptors.dart
- M tests/dartdevc/debugger/debugger_test_golden.txt
- M tests/dartdevc/libraries_test.dart
- A tests/hot_reload/cross_module_super/lib.0.dart
- A tests/hot_reload/cross_module_super/main.0.dart
- A tests/hot_reload/tear_off_super_getter/main.0.dart
- A tests/hot_reload/tear_off_super_getter/main.1.dart
Change size: L
Delta: 16 files changed, 608 insertions(+), 175 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Nicholas Shahan, +1 by Nate Biggs
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages