-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Overhaul import resolution #19391
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
eb05996
Move getAChildContainer one scope up
asgerf 2ce01bf
Add Folder::Resolve as a generalisation of Folder::Append
asgerf ec9d15b
JS: Make shared Folder module visible
asgerf 8c0b0c4
JS: Ensure json files are extracted properly in tests
asgerf 359525b
JS: Extract more tsconfig.json patterns
asgerf 565cb43
JS: Add test
asgerf 17aa522
JS: Add some helpers
asgerf ef32a03
JS: Extract from methods from PathString into a non-abstract base class
asgerf 59e1cbc
JS: Add tsconfig class
asgerf bb91df8
JS: Add helper for doing path resolution with JS rules
asgerf f542956
JS: Add internal extension of PackageJson class
asgerf ed4864e
JS: Add two more helpers to FilePath class
asgerf 6725cb5
JS: Implement import resolution
asgerf e4420f6
JS: Move babel-root-import test
asgerf d724874
JS: Implement babel-plugin-root-import as a PathMapping
asgerf a195d07
JS: Resolve Angular2 templateUrl with ResolveExpr instead of PathExpr
asgerf c293f03
JS: Remove a dependency on getImportedPath()
asgerf fe055ad
JS: Use PackageJsonEx instead of resolveMainModule
asgerf ed2a832
JS: Deprecate PathExpr and related classes
asgerf be5de9c
JS: Update test output
asgerf 5de2c93
JS: Rename getTargetFile to getImportedFile and remove its deprecated…
asgerf 70a5ec5
JS: Add package.json files in tests relying on node_modules
asgerf b0f73f1
JS: Update test output now that we import .d.ts files more liberally
asgerf f3e0cfd
Apply suggestions from code review
asgerf 5c9218f
JS: Add comment about 'path' heuristic
asgerf 1f308ee
JS: Explain use of monotonicAggregates
asgerf 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
JS: Add internal extension of PackageJson class
- Loading branch information
commit f542956f66d930be53e7b3134b39a251bbdb354c
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
151 changes: 151 additions & 0 deletions
151
javascript/ql/lib/semmle/javascript/internal/paths/PackageJsonEx.qll
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,151 @@ | ||
private import javascript | ||
private import semmle.javascript.internal.paths.JSPaths | ||
|
||
/** | ||
* Extension of `PackageJson` with some internal path-resolution predicates. | ||
*/ | ||
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved
Hide resolved
|
||
class PackageJsonEx extends PackageJson { | ||
private JsonValue getAPartOfExportsSection(string pattern) { | ||
result = this.getPropValue("exports") and | ||
pattern = "" | ||
or | ||
exists(string prop, string prevPath | | ||
result = this.getAPartOfExportsSection(prevPath).getPropValue(prop) and | ||
if prop.matches("./%") then pattern = prop.suffix(2) else pattern = prevPath | ||
) | ||
} | ||
|
||
predicate hasPathMapping(string pattern, string newPath) { | ||
this.getAPartOfExportsSection(pattern).getStringValue() = newPath | ||
} | ||
|
||
predicate hasExactPathMapping(string pattern, string newPath) { | ||
this.getAPartOfExportsSection(pattern).getStringValue() = newPath and | ||
not pattern.matches("%*%") | ||
} | ||
|
||
predicate hasPrefixPathMapping(string pattern, string newPath) { | ||
this.hasPathMapping(pattern + "*", newPath + "*") | ||
} | ||
|
||
predicate hasExactPathMappingTo(string pattern, Container target) { | ||
exists(string newPath | | ||
this.hasExactPathMapping(pattern, newPath) and | ||
target = Resolver::resolve(this.getFolder(), newPath) | ||
) | ||
} | ||
|
||
predicate hasPrefixPathMappingTo(string pattern, Container target) { | ||
exists(string newPath | | ||
this.hasPrefixPathMapping(pattern, newPath) and | ||
target = Resolver::resolve(this.getFolder(), newPath) | ||
) | ||
} | ||
|
||
string getMainPath() { result = this.getPropStringValue(["main", "module"]) } | ||
|
||
File getMainFile() { | ||
exists(Container main | main = Resolver::resolve(this.getFolder(), this.getMainPath()) | | ||
result = main | ||
or | ||
result = main.(Folder).getJavaScriptFileOrTypings("index") | ||
) | ||
} | ||
|
||
File getMainFileOrBestGuess() { | ||
result = this.getMainFile() | ||
or | ||
result = guessPackageJsonMain1(this) | ||
or | ||
result = guessPackageJsonMain2(this) | ||
} | ||
|
||
string getAPathInFilesArray() { | ||
result = this.getPropValue("files").(JsonArray).getElementStringValue(_) | ||
} | ||
|
||
Container getAFileInFilesArray() { | ||
result = Resolver::resolve(this.getFolder(), this.getAPathInFilesArray()) | ||
} | ||
|
||
override File getTypingsFile() { | ||
result = Resolver::resolve(this.getFolder(), this.getTypings()) | ||
or | ||
not exists(this.getTypings()) and | ||
exists(File mainFile | | ||
mainFile = this.getMainFileOrBestGuess() and | ||
result = | ||
mainFile | ||
.getParentContainer() | ||
.getFile(mainFile.getStem().regexpReplaceAll("\\.d$", "") + ".d.ts") | ||
) | ||
} | ||
} | ||
|
||
private module ResolverConfig implements Folder::ResolveSig { | ||
additional predicate shouldResolve(PackageJsonEx pkg, Container base, string path) { | ||
base = pkg.getFolder() and | ||
( | ||
pkg.hasExactPathMapping(_, path) | ||
or | ||
pkg.hasPrefixPathMapping(_, path) | ||
or | ||
path = pkg.getMainPath() | ||
or | ||
path = pkg.getAPathInFilesArray() | ||
or | ||
path = pkg.getTypings() | ||
) | ||
} | ||
|
||
predicate shouldResolve(Container base, string path) { shouldResolve(_, base, path) } | ||
|
||
predicate getAnAdditionalChild = JSPaths::getAnAdditionalChild/2; | ||
|
||
predicate isOptionalPathComponent(string segment) { | ||
// Try to omit paths can might refer to a build format, .e.g `dist/cjs/foo.cjs` -> `src/foo.ts` | ||
segment = ["cjs", "mjs", "js"] | ||
} | ||
|
||
bindingset[segment] | ||
string rewritePathSegment(string segment) { | ||
// Try removing anything after the first dot, such as foo.min.js -> foo (the extension is then filled in by getAdditionalChild) | ||
result = segment.regexpReplaceAll("\\..*", "") | ||
} | ||
} | ||
|
||
private module Resolver = Folder::Resolve<ResolverConfig>; | ||
|
||
/** | ||
* Removes the scope from a package name, e.g. `@foo/bar` -> `bar`. | ||
*/ | ||
bindingset[name] | ||
private string stripPackageScope(string name) { result = name.regexpReplaceAll("^@[^/]+/", "") } | ||
|
||
private predicate isImplementationFile(File f) { not f.getBaseName().matches("%.d.ts") } | ||
|
||
File guessPackageJsonMain1(PackageJsonEx pkg) { | ||
not isImplementationFile(pkg.getMainFile()) and | ||
exists(Folder folder, Folder subfolder | | ||
folder = pkg.getFolder() and | ||
( | ||
subfolder = folder or | ||
subfolder = folder.getChildContainer(getASrcFolderName()) or | ||
subfolder = | ||
folder | ||
.getChildContainer(getASrcFolderName()) | ||
.(Folder) | ||
.getChildContainer(getASrcFolderName()) | ||
) | ||
| | ||
result = subfolder.getJavaScriptFileOrTypings("index") | ||
or | ||
result = subfolder.getJavaScriptFileOrTypings(stripPackageScope(pkg.getDeclaredPackageName())) | ||
) | ||
} | ||
|
||
File guessPackageJsonMain2(PackageJsonEx pkg) { | ||
not isImplementationFile(pkg.getMainFile()) and | ||
not isImplementationFile(guessPackageJsonMain1(pkg)) and | ||
result = pkg.getAFileInFilesArray() | ||
} |
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.
Can we delete this predicate now? It feel wrong to leave a
none()
predicate like this.I know that it's never actually
none()
, because it's always overriden inPackageJsonEx
.But, in general I don't like the
class *Ex
pattern, it feels like it's hiding the real implementation.Could you instead have the internal path resolution in e.g. a private module?
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.
It may not look pretty but honestly I just don't think it's worth the hassle to start moving and renaming things in order to make it possible to move the implementation in here. The
none()
with override is a fairly well-understood way to keep the implementation in a different file, where we have access to the parts we need.