library.importUri.path == 'meta/meta.dart');Daco HarkesShould not be a special case - just add entry point pragmas for the RecordUse class in that library.
Alexander MarkovUnfortunately, that doesn't work. There are already package published out there without a pragma.
(Honestly, I don't think this annotation should be in a package, it should be in the SDK. But that's a different discussion to have.)
We can change the package and re-publish. Given that the recording of usages is a new, not yet released feature, its okay if it would require a fairly new package version to work.
Also note that compiler *should not* recognize and specially handle some specific class in a third-party package. While we could move `@RecordUse` annotation to the Dart SDK, a better way would be to introduce a `pragma('record-use')` and define a
```
const RecordUse = pragma('record-use');
```
in a third-party package if needed (this declaration can be used as `@RecordUse`). Our compilers can recognize pragmas and work accordingly without any dependencies on package:meta. This would also remove the need to retain `RecordUse` class (`pragma` is already retained).| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
klass.members.any(record_use.isBeingRecorded)) {Why is this needed? If member is being recorded, then it will be retained and its enclosing class will be retained as well, without additional handling.
nativeCodeOracle.isClassReferencedFromNativeCode(Could you explain this change? In general, if class has external usages it does not mean we need to retain all its fields. So this logic should be applies only to the classes being recorded, right?
.isMemberReferencedFromNativeCode(member)) {Is this still needed? I think signature shaking of members referenced externally is disabled at L268-269.
!shaker.isClassReferencedFromNativeCode(node)) {The extra condition is not needed, `isClassReferencedFromNativeCode` is already included into `isClassUsed` at this point.
!shaker.isClassReferencedFromNativeCode(node)) {ditto, also below
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
klass.members.any(record_use.isBeingRecorded)) {Why is this needed? If member is being recorded, then it will be retained and its enclosing class will be retained as well, without additional handling.
It's to prevent marking enums as classes in
```
if (!shaker.isClassAllocated(node) &&
!shaker.isClassReferencedFromNativeCode(node)) {
// Prevent TFA from making the class abstract or changing its enum status
// if itself or its members are recorded. If it becomes abstract or loses
// its enum status, record_use might report it incorrectly (e.g. as a
// class instead of an enum).
debugPrint('Class ${node.name} converted to abstract');
node.isAbstract = true;
node.isEnum = false;
}
```
Added more comments.
Could you explain this change? In general, if class has external usages it does not mean we need to retain all its fields. So this logic should be applies only to the classes being recorded, right?
Done
library.importUri.path == 'meta/meta.dart');Daco HarkesShould not be a special case - just add entry point pragmas for the RecordUse class in that library.
Alexander MarkovUnfortunately, that doesn't work. There are already package published out there without a pragma.
(Honestly, I don't think this annotation should be in a package, it should be in the SDK. But that's a different discussion to have.)
We can change the package and re-publish. Given that the recording of usages is a new, not yet released feature, its okay if it would require a fairly new package version to work.
Also note that compiler *should not* recognize and specially handle some specific class in a third-party package. While we could move `@RecordUse` annotation to the Dart SDK, a better way would be to introduce a `pragma('record-use')` and define a
```
const RecordUse = pragma('record-use');
```
in a third-party package if needed (this declaration can be used as `@RecordUse`). Our compilers can recognize pragmas and work accordingly without any dependencies on package:meta. This would also remove the need to retain `RecordUse` class (`pragma` is already retained).
We can change the package and re-publish. Given that the recording of usages is a new, not yet released feature, its okay if it would require a fairly new package version to work.
Good point!
---
Yeah, I also have the feeling that it should live in the SDK: https://github.com/dart-lang/native/issues/2680
Pragma's would indeed be nice. It fits very well with the fact that it's a compiler feature. However, we'd also lose the `@Target` analyzer support that we have for custom annotations. And PMs & devrel might have opinions about the API from a holistic POV. I'll revisit this when we get closer to the final API & user journeys.
For now, I'll add the pragma in meta.
Is this still needed? I think signature shaking of members referenced externally is disabled at L268-269.
Done
The extra condition is not needed, `isClassReferencedFromNativeCode` is already included into `isClassUsed` at this point.
Done
!shaker.isClassReferencedFromNativeCode(node)) {Daco Harkesditto, also below
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
klass.members.any(record_use.isBeingRecorded)) {Daco HarkesWhy is this needed? If member is being recorded, then it will be retained and its enclosing class will be retained as well, without additional handling.
It's to prevent marking enums as classes in
```
if (!shaker.isClassAllocated(node) &&
!shaker.isClassReferencedFromNativeCode(node)) {
// Prevent TFA from making the class abstract or changing its enum status
// if itself or its members are recorded. If it becomes abstract or loses
// its enum status, record_use might report it incorrectly (e.g. as a
// class instead of an enum).
debugPrint('Class ${node.name} converted to abstract');
node.isAbstract = true;
node.isEnum = false;
}
```Added more comments.
This would only happen if there are no instances allocated, e.g. there are no uses of enum values. Why do we need to report any usages in such a case? Also, if enum element is marked as setMemberReferencedFromNativeCode, then its value is retained and enum class would be marked as isClassAllocated because constant value of that enum element is an instance of the enum class.
!shaker.isClassReferencedFromNativeCode(node)) {If !shaker.isClassAllocated, we should not report any usages of the enum, so this should not be needed.
!shaker.isMemberReferencedFromNativeCode(node)) {Not needed as isMemberReferencedFromNativeCode members should be already added to isMemberUsed at this point. Also note that we cannot decide to retain a member here (during shaker pass 2) as it is a much more complicated process, see TreeShaker.addUsedMember which is called during shaker pass 1.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
klass.members.any(record_use.isBeingRecorded)) {Daco HarkesWhy is this needed? If member is being recorded, then it will be retained and its enclosing class will be retained as well, without additional handling.
Alexander MarkovIt's to prevent marking enums as classes in
```
if (!shaker.isClassAllocated(node) &&
!shaker.isClassReferencedFromNativeCode(node)) {
// Prevent TFA from making the class abstract or changing its enum status
// if itself or its members are recorded. If it becomes abstract or loses
// its enum status, record_use might report it incorrectly (e.g. as a
// class instead of an enum).
debugPrint('Class ${node.name} converted to abstract');
node.isAbstract = true;
node.isEnum = false;
}
```Added more comments.
This would only happen if there are no instances allocated, e.g. there are no uses of enum values. Why do we need to report any usages in such a case? Also, if enum element is marked as setMemberReferencedFromNativeCode, then its value is retained and enum class would be marked as isClassAllocated because constant value of that enum element is an instance of the enum class.
No it's also about static methods being recorded in enums. The parent of those methods is then falsely reported as aclass.
!shaker.isClassReferencedFromNativeCode(node)) {If !shaker.isClassAllocated, we should not report any usages of the enum, so this should not be needed.
This is for static methods on enhanced enums that need to have their parent reported as an enum.
!shaker.isMemberReferencedFromNativeCode(node)) {Not needed as isMemberReferencedFromNativeCode members should be already added to isMemberUsed at this point. Also note that we cannot decide to retain a member here (during shaker pass 2) as it is a much more complicated process, see TreeShaker.addUsedMember which is called during shaker pass 1.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
klass.members.any(record_use.isBeingRecorded)) {Daco HarkesWhy is this needed? If member is being recorded, then it will be retained and its enclosing class will be retained as well, without additional handling.
Alexander MarkovIt's to prevent marking enums as classes in
```
if (!shaker.isClassAllocated(node) &&
!shaker.isClassReferencedFromNativeCode(node)) {
// Prevent TFA from making the class abstract or changing its enum status
// if itself or its members are recorded. If it becomes abstract or loses
// its enum status, record_use might report it incorrectly (e.g. as a
// class instead of an enum).
debugPrint('Class ${node.name} converted to abstract');
node.isAbstract = true;
node.isEnum = false;
}
```Added more comments.
Daco HarkesThis would only happen if there are no instances allocated, e.g. there are no uses of enum values. Why do we need to report any usages in such a case? Also, if enum element is marked as setMemberReferencedFromNativeCode, then its value is retained and enum class would be marked as isClassAllocated because constant value of that enum element is an instance of the enum class.
No it's also about static methods being recorded in enums. The parent of those methods is then falsely reported as aclass.
I had this in an earlier patchset:
But we were worried that having an abstract enum would be a combination of things that could lead to issues downstream. So then we disabled the whole optimization.
klass.members.any(record_use.isBeingRecorded)) {Daco HarkesWhy is this needed? If member is being recorded, then it will be retained and its enclosing class will be retained as well, without additional handling.
Alexander MarkovIt's to prevent marking enums as classes in
```
if (!shaker.isClassAllocated(node) &&
!shaker.isClassReferencedFromNativeCode(node)) {
// Prevent TFA from making the class abstract or changing its enum status
// if itself or its members are recorded. If it becomes abstract or loses
// its enum status, record_use might report it incorrectly (e.g. as a
// class instead of an enum).
debugPrint('Class ${node.name} converted to abstract');
node.isAbstract = true;
node.isEnum = false;
}
```Added more comments.
Daco HarkesThis would only happen if there are no instances allocated, e.g. there are no uses of enum values. Why do we need to report any usages in such a case? Also, if enum element is marked as setMemberReferencedFromNativeCode, then its value is retained and enum class would be marked as isClassAllocated because constant value of that enum element is an instance of the enum class.
No it's also about static methods being recorded in enums. The parent of those methods is then falsely reported as aclass.
Why is it important for users to distinguish enums and classes when usages of a static method are reported? Can we just report that Foo.bar is used, regardless of Foo being a class or enum?
If you feel strongly about that, consider moving this logic to handling of members to avoid duplicate checks (we visit members anyway), something along the lines:
```
visitProcedure(Procedure proc) {
if (record_use.isBeingRecorded(proc)) {
nativeCodeOracle.setMemberReferencedFromNativeCode(proc);
final enclosingClass = proc.enclosingClass;
if (enclosingClass != null) {
// Do not transform enclosing class to guarantee accurate reporting of usages.
nativeCodeOracle.addClassReferencedFromNativeCode(enclosingClass);
}
}
```
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
In the current way the API is set up, a class with a name is not the same as the enum with that name. And the API has only have one way to represent `Definition`s. We don't want to conflate enum and class instances. And, I'd also like to avoid using a different classes for `Definition`s.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
I assume you mean third_party/pkg/native/pkgs/record_use/lib/src/definition.dart API?
Maybe I'm missing something, but what is the use case for providing `DefinitionKind` in the record use API? Say, if you're interested in all usages of `IconData`, why do you need to care if it is a class or enum? You actually know what you're looking for and what it is. As for the name conflicts, I don't think they can happen as classes and enums share the same name space. The only case which might potentially need disambiguation is setters, and we can follow Dart language specification which solves this via "The name of a setter is obtained by appending the string ā=ā to the identifier given in its signature.").
I keep questioning this because it seems like it adds unnecessary complexity and/or inefficiency without a good reason (without benefiting users). The remaining concern is related to the additional `!shaker.isClassReferencedFromNativeCode(node)` condition in the tree shaker. We should not really regress how we do tree shaking if recording of usages is not used at all (and when it is used we should only regress it for classes and members for which usages where requested). So, if you insist on keeping this API, then we should probably have a separate Set<Class> in the NatevCodeOracle, something like `_classesWithPersistentShape`/`isClassWithPersistentShape`/`addClassesWithPersistentShape`, add enclosing class of members being recorded and consult with it in tree shaker before turning enum into a class.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yes, indeed that's the API.
You might be right about it not having a use case right now, but I don't want to set up this format that's untrue w.r.t. the Dart language specification. I don't want to lump classes and enums into the same kind. And I also don't want to simply remove the kind for class/enum but keep it for other things.
A agree that this is complexity. But all of the 'preserving' or 'reconstructing' the source name and kind is the same complexity in all of the kernel lowering_predicates and TFA optimization disabling. (We also recover the getter/setter kinds for example.)
I agree that having `_classesWithPersistentShape` sounds like a cleaner solution than what I did now. I shoehorned a bit too much into the existing API.
I'll take a look at this on Monday.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
klass.members.any(record_use.isBeingRecorded)) {Done
!shaker.isClassReferencedFromNativeCode(node)) {Daco HarkesIf !shaker.isClassAllocated, we should not report any usages of the enum, so this should not be needed.
This is for static methods on enhanced enums that need to have their parent reported as an enum.
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
[record_use][tfa] Moving `isBeingUsed` to `NativeCodeOracle`
TEST=pkg/compiler/test/record_use/record_use_test.dart
TEST=pkg/dart2wasm/test/record_use_test.dart
TEST=pkg/vm/test/transformations/record_use_test.dart
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |