Skip to content

JS: refactor get predicates with no return #18187

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

JS: refactor get predicates with no return #18187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eca04ff
JS: refactor getADependency to return a string
Napalys Dec 3, 2024
1a6f1f5
JS: Refactor Test Cases - 'get' Predicates Without Return Values
Napalys Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions javascript/ql/lib/semmle/javascript/NPM.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class PackageJson extends JsonObject {
* different from the other dependency types.
*/
predicate declaresDependency(string pkg, string version) {
this.getADependenciesObject(_).getADependency(pkg, version)
this.getADependenciesObject(_).getADependency(pkg) = version
}

/** Gets the engine dependencies of this package. */
Expand Down Expand Up @@ -340,8 +340,8 @@ class PackageDependencies extends JsonObject {
)
}

/** Holds if this package depends on version 'version' of package 'pkg'. */
predicate getADependency(string pkg, string version) { version = this.getPropStringValue(pkg) }
/** Returns the version of the specified package that this package depends on. */
string getADependency(string pkg) { result = this.getPropStringValue(pkg) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is in draft PR state, but I a peek anyway.

We have to be wary of breaking changes in predicates that are part of the public API. To change the signature of this predicate, we'd have to rename it and leave behind a deprecated version of the original predicate.

Apart from that, I think I'd prefer to rename it to hasDependency instead, and not add a return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if we rename it to hasDependency, we would still need to keep the original predicate getADependency marked as deprecated and in the comment refer that people now should use hasDependency, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's right

}

/**
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/lib/semmle/javascript/frameworks/Next.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module NextJS {
/**
* Gets a `package.json` that depends on the `Next.js` library.
*/
PackageJson getANextPackage() { result.getDependencies().getADependency("next", _) }
PackageJson getANextPackage() { exists(result.getDependencies().getADependency("next")) }

/**
* Gets a "pages" folder in a `Next.js` application.
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/lib/semmle/javascript/frameworks/Redux.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module Redux {
}

private predicate packageDependsOn(PackageJson importer, PackageJson dependency) {
importer.getADependenciesObject("").getADependency(dependency.getPackageName(), _)
exists(importer.getADependenciesObject("").getADependency(dependency.getPackageName()))
}

/** Gets a package that can be considered an entry point for a Redux app. */
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/lib/semmle/javascript/frameworks/Templating.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ module Templating {

private TemplateSyntax getOwnTemplateSyntaxInFolder(Folder f) {
exists(PackageDependencies deps |
deps.getADependency(result.getAPackageName(), _) and
exists(deps.getADependency(result.getAPackageName())) and
f = deps.getFile().getParentContainer()
)
}
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/lib/semmle/javascript/frameworks/Vuex.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ module Vuex {
}

private predicate packageDependsOn(PackageJson importer, PackageJson dependency) {
importer.getADependenciesObject("").getADependency(dependency.getPackageName(), _)
exists(importer.getADependenciesObject("").getADependency(dependency.getPackageName()))
}

/** Gets a package that can be considered an entry point for a Vuex app. */
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/src/NodeJS/UnresolvableImport.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ where
pkg.getAModule() = r.getTopLevel() and pkgJson = pkg.getPackageJson()
|
not pkgJson.declaresDependency(mod, _) and
not pkgJson.getPeerDependencies().getADependency(mod, _) and
not exists(pkgJson.getPeerDependencies().getADependency(mod)) and
// exclude packages depending on `fbjs`, which automatically pulls in many otherwise
// undeclared dependencies
not pkgJson.declaresDependency("fbjs", _)
Expand Down
8 changes: 4 additions & 4 deletions javascript/ql/test/library-tests/ClassNode/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ query predicate fieldStep(DataFlow::Node pred, DataFlow::Node succ) {
DataFlow::localFieldStep(pred, succ)
}

query predicate getAReceiverNode(DataFlow::ClassNode cls, DataFlow::SourceNode recv) {
cls.getAReceiverNode() = recv
query DataFlow::SourceNode getAReceiverNode(DataFlow::ClassNode cls) {
result = cls.getAReceiverNode()
}

query predicate getFieldTypeAnnotation(DataFlow::ClassNode cls, string name, TypeAnnotation ann) {
ann = cls.getFieldTypeAnnotation(name)
query TypeAnnotation getFieldTypeAnnotation(DataFlow::ClassNode cls, string name) {
result = cls.getFieldTypeAnnotation(name)
}

query predicate instanceMember(
Expand Down
6 changes: 2 additions & 4 deletions javascript/ql/test/library-tests/Comprehensions/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@ query predicate comprehensionExpr(ComprehensionExpr ce, int numBlock, int numFil
body = ce.getBody()
}

query predicate getBlock(ComprehensionExpr ce, int i, ComprehensionBlock block) {
ce.getBlock(i) = block
}
query ComprehensionBlock getBlock(ComprehensionExpr ce, int i) { result = ce.getBlock(i) }

query predicate getFilter(ComprehensionExpr ce, int i, Expr filter) { ce.getFilter(i) = filter }
query Expr getFilter(ComprehensionExpr ce, int i) { result = ce.getFilter(i) }

query predicate varDecls(VarAccess va, VarDecl decl) { decl = va.getVariable().getADeclaration() }
6 changes: 3 additions & 3 deletions javascript/ql/test/library-tests/DataFlow/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ query predicate enclosingExpr(DataFlow::Node node, Expr enclosingExpr) {

query predicate flowStep(DataFlow::Node pred, DataFlow::Node nd) { nd.getAPredecessor() = pred }

query predicate getImmediatePredecessor(DataFlow::Node pred, DataFlow::Node nd) {
nd.getImmediatePredecessor() = pred
query DataFlow::Node getImmediatePredecessor(DataFlow::Node pred) {
result.getImmediatePredecessor() = pred
}

query predicate getIntValue(DataFlow::Node node, int val) { node.getIntValue() = val }
query int getIntValue(DataFlow::Node node) { result = node.getIntValue() }

query predicate incomplete(DataFlow::Node dfn, DataFlow::Incompleteness cause) {
dfn.isIncomplete(cause)
Expand Down
4 changes: 1 addition & 3 deletions javascript/ql/test/library-tests/Flow/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ query predicate abseval(

query predicate abstractValues(AbstractValue val) { any() }

query predicate getAPrototype(AbstractValue av, DefiniteAbstractValue proto) {
av.getAPrototype() = proto
}
query DefiniteAbstractValue getAPrototype(AbstractValue av) { result = av.getAPrototype() }

private import semmle.javascript.dataflow.Refinements

Expand Down
4 changes: 2 additions & 2 deletions javascript/ql/test/library-tests/NPM/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ query predicate npm(PackageJson pkg, string name, string version) {
version = pkg.getVersion()
}

query predicate getMainModule(PackageJson pkg, string name, Module mod) {
query Module getMainModule(PackageJson pkg, string name) {
name = pkg.getPackageName() and
mod = pkg.getMainModule()
result = pkg.getMainModule()
}

query predicate packageJson(PackageJson json) { any() }
Expand Down
6 changes: 3 additions & 3 deletions javascript/ql/test/library-tests/TypeScript/Types/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import javascript
// Ensure `true | false` and `false | true` are not distinct boolean types.
query predicate booleans(BooleanType t) { any() }

query predicate getExprType(Expr expr, Type type) { type = expr.getType() }
query Type getExprType(Expr expr) { result = expr.getType() }

query predicate getTypeDefinitionType(TypeDefinition def, Type type) { type = def.getType() }
query Type getTypeDefinitionType(TypeDefinition def) { result = def.getType() }

query predicate getTypeExprType(TypeExpr e, Type type) { e.getType() = type }
query Type getTypeExprType(TypeExpr e) { result = e.getType() }

query predicate missingToString(Type typ, string msg) {
not exists(typ.toString()) and
Expand Down
7 changes: 3 additions & 4 deletions javascript/ql/test/library-tests/frameworks/Express/RouteHandlerContainer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import javascript

query predicate getRouteHandlerContainerStep(
Http::RouteHandlerCandidateContainer container, DataFlow::SourceNode handler,
DataFlow::SourceNode access
query DataFlow::SourceNode getRouteHandlerContainerStep(
Http::RouteHandlerCandidateContainer container, DataFlow::SourceNode handler
) {
handler = container.getRouteHandler(access)
handler = container.getRouteHandler(result)
}
6 changes: 2 additions & 4 deletions javascript/ql/test/library-tests/variables/tests.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@ query predicate capture(LocalVariable var, string name, VarDecl decl) {
var.getADeclaration() = decl and name = var.getName()
}

query predicate getAnAssignedExpr(Variable v, Expr e) { e = v.getAnAssignedExpr() }
query Expr getAnAssignedExpr(Variable v) { result = v.getAnAssignedExpr() }

query predicate getDeclaringContainer(LocalVariable v, StmtContainer container) {
container = v.getDeclaringContainer()
}
query StmtContainer getDeclaringContainer(LocalVariable v) { result = v.getDeclaringContainer() }

query predicate varBindings(VarAccess va, VarDecl decl) {
decl = va.getVariable().getADeclaration()
Expand Down
2 changes: 1 addition & 1 deletion javascript/ql/test/tutorials/Introducing the JavaScript libraries/query19.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import javascript
query predicate test_query19(PackageDependencies deps, string res) {
exists(NpmPackage pkg, string name |
deps = pkg.getPackageJson().getDependencies() and
deps.getADependency(name, _) and
exists(deps.getADependency(name)) and
not exists(Require req | req.getTopLevel() = pkg.getAModule() |
name = req.getImportedPath().getValue()
)
Expand Down
Loading