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

[vm_service] FieldRef.library.uri points to the wrong library for mixins #45093

Closed
rrousselGit opened this issue Feb 23, 2021 · 7 comments
Closed
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. vm-debugger

Comments

@rrousselGit
Copy link

rrousselGit commented Feb 23, 2021

Consider:

// a.dart

mixin Mixin {
  int _private = 42;
}

// b.dart

class Class with Mixin {
  int _private = 42;
}

When iterating over Instance.fields on a Class instance, this will correctly report two _private fields but:
Mixin._private's FieldRef.owner.uri will point to b.dart instead of a.dart

This is problematic because that can cause evaluations to be performed on Class._private instead of Mixin._private.
An example of such evaluation could be classInstance._private++

@jacob314
Copy link
Member

This is a tricky edge case. Good news there is a way out without any VM (or more accurately mixin kernel transformer changes)
The sourceLocation from a BoundField should tell you the library the private symbol is private relative to.
https://github.com/dart-lang/sdk/blob/master/runtime/vm/service/service.md#field
https://github.com/dart-lang/sdk/blob/master/runtime/vm/service/service.md#sourcelocation

@jacob314
Copy link
Member

Fyi @bkonyi

@mit-mit mit-mit added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Feb 24, 2021
@bkonyi bkonyi self-assigned this Mar 30, 2021
@bkonyi
Copy link
Contributor

bkonyi commented Apr 6, 2022

After talking with @rmacnak-google, I think this is WAI as the mixed in field is owned by the class which the mixin was applied to, not the original mixin declaration. I've gone ahead and updated the documentation here to explain some of the situations where the location of owner may not match the location of a field/function's origin.

@bkonyi bkonyi closed this as completed Apr 6, 2022
copybara-service bot pushed a commit that referenced this issue Apr 7, 2022
Clears up confusion around function/field owner locations not
necessarily representing where the function/field was actually declared
(e.g., for functions and fields brought into a class via a mixin
application).

Fixes #45093

TEST=Documentation change

Change-Id: Ideaf17ec99d005459c60a2dd88f72b3485b32664
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240481
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
@rrousselGit
Copy link
Author

@bkonyi Unless I'm missing something, I don't agree with this getting closed.

The root of the issue is that this behavior prevents mutating private properties of mixins as defined in the issue.

To make the expression mixin._property = value valid, we need to know the URI in which the mixin is defined – which we can't do because of this issue.

@bkonyi
Copy link
Contributor

bkonyi commented Apr 11, 2022

It's still possible to get access to the declaration location of the properties from their sourceLocation, which is more accurate than going off of the location of the property's owner. In general, sourceLocation of a service object should be used to determine where the corresponding property was declared.

@rrousselGit
Copy link
Author

I see, thanks for sharing. I didn't know about this property

@bkonyi
Copy link
Contributor

bkonyi commented Apr 11, 2022

Not a problem! :-)

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-debugger
Projects
None yet
Development

No branches or pull requests

4 participants