This repository was archived by the owner on Feb 25, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 6k
Const finder fixes #15880
Merged
Merged
Const finder fixes #15880
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
09240e4
Fix issues in const_finder
dnfield 6764323
test
dnfield 2f7c065
void return
dnfield b3a6d4a
more
dnfield 3f869e2
test more cases
dnfield 61b695b
Merge remote-tracking branch 'upstream/master' into const_finder_fixes
dnfield 0d6040d
test pkg
dnfield 340710f
..
dnfield File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,28 +8,25 @@ import 'package:meta/meta.dart'; | |
class _ConstVisitor extends RecursiveVisitor<void> { | ||
_ConstVisitor( | ||
this.kernelFilePath, | ||
this.targetLibraryUri, | ||
this.classLibraryUri, | ||
this.className, | ||
) : assert(kernelFilePath != null), | ||
assert(targetLibraryUri != null), | ||
assert(classLibraryUri != null), | ||
assert(className != null), | ||
_visitedInstnaces = <String>{}, | ||
constantInstances = <Map<String, dynamic>>[], | ||
nonConstantLocations = <Map<String, dynamic>>[]; | ||
|
||
/// The path to the file to open. | ||
final String kernelFilePath; | ||
|
||
/// The library URI for the main entrypoint of the target library. | ||
final String targetLibraryUri; | ||
|
||
/// The library URI for the class to find. | ||
final String classLibraryUri; | ||
|
||
/// The name of the class to find. | ||
final String className; | ||
|
||
final Set<String> _visitedInstnaces; | ||
final List<Map<String, dynamic>> constantInstances; | ||
final List<Map<String, dynamic>> nonConstantLocations; | ||
|
||
|
@@ -43,6 +40,7 @@ class _ConstVisitor extends RecursiveVisitor<void> { | |
final Class parentClass = node.target.parent as Class; | ||
if (!_matches(parentClass)) { | ||
super.visitConstructorInvocation(node); | ||
return; | ||
} | ||
nonConstantLocations.add(<String, dynamic>{ | ||
'file': node.location.file.toString(), | ||
|
@@ -53,15 +51,22 @@ class _ConstVisitor extends RecursiveVisitor<void> { | |
|
||
@override | ||
void visitInstanceConstantReference(InstanceConstant node) { | ||
node.fieldValues.values.whereType<InstanceConstant>().forEach(visitInstanceConstantReference); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is important if the const is defined inline somewhere - it never gets visited by this method otherwise. It has to be up here before we check for matches because it could either be in the class we care about or in some other class. |
||
if (!_matches(node.classNode)) { | ||
super.visitInstanceConstantReference(node); | ||
return; | ||
} | ||
final Map<String, dynamic> instance = <String, dynamic>{}; | ||
for (MapEntry<Reference, Constant> kvp in node.fieldValues.entries) { | ||
if (kvp.value is! PrimitiveConstant<dynamic>) { | ||
continue; | ||
} | ||
final PrimitiveConstant<dynamic> value = kvp.value as PrimitiveConstant<dynamic>; | ||
instance[kvp.key.canonicalName.name] = value.value; | ||
} | ||
constantInstances.add(instance); | ||
if (_visitedInstnaces.add(instance.toString())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is cheating a little, but it's simpler than creating a class that can properly hash and equality compare a dynamic map. It's good enough to dedup things when all the parameters are in order. |
||
constantInstances.add(instance); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -71,53 +76,27 @@ class ConstFinder { | |
/// be null. | ||
/// | ||
/// The `kernelFilePath` is the path to a dill (kernel) file to process. | ||
/// | ||
/// The `targetLibraryUri` is the `package:` URI of the main entrypoint to | ||
/// search from. | ||
/// | ||
/// | ||
/// | ||
ConstFinder({ | ||
@required String kernelFilePath, | ||
@required String targetLibraryUri, | ||
@required String classLibraryUri, | ||
@required String className, | ||
}) : _visitor = _ConstVisitor( | ||
kernelFilePath, | ||
targetLibraryUri, | ||
classLibraryUri, | ||
className, | ||
); | ||
|
||
final _ConstVisitor _visitor; | ||
|
||
Library _getRoot() { | ||
final Component binary = loadComponentFromBinary(_visitor.kernelFilePath); | ||
return binary.libraries.firstWhere( | ||
(Library library) => library.canonicalName.name == _visitor.targetLibraryUri, | ||
orElse: () => throw LibraryNotFoundException._(_visitor.targetLibraryUri), | ||
); | ||
} | ||
|
||
/// Finds all instances | ||
Map<String, dynamic> findInstances() { | ||
final Library root = _getRoot(); | ||
root.visitChildren(_visitor); | ||
_visitor._visitedInstnaces.clear(); | ||
for (Library library in loadComponentFromBinary(_visitor.kernelFilePath).libraries) { | ||
library.visitChildren(_visitor); | ||
} | ||
return <String, dynamic>{ | ||
'constantInstances': _visitor.constantInstances, | ||
'nonConstantLocations': _visitor.nonConstantLocations, | ||
}; | ||
} | ||
} | ||
|
||
/// Exception thrown by [ConstFinder.findInstances] when the target library | ||
/// is not found. | ||
class LibraryNotFoundException implements Exception { | ||
const LibraryNotFoundException._(this.targetLibraryUri); | ||
|
||
/// The library target URI that could not be found. | ||
final String targetLibraryUri; | ||
|
||
@override | ||
String toString() => 'Could not find target library for "$targetLibraryUri".'; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
# Generated by pub on 2020-01-15 10:08:29.776333. | ||
const_finder_fixtures:lib/ | ||
const_finder_fixtures_package:pkg/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import 'package:const_finder_fixtures/target.dart'; | ||
|
||
void createTargetInPackage() { | ||
const Target target = Target('package', -1, null); | ||
target.hit(); | ||
} | ||
|
||
void createNonConstTargetInPackage() { | ||
// ignore: prefer_const_constructors | ||
final Target target = Target('package_non', -2, null); | ||
target.hit(); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just doesn't work. Gallery, for example, uses multiple libraries to define icons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add any tests to cover this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're making me sad. I'll update the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done