Skip to content

JS: recognize transform-react-jsx plugin#373

Merged
semmle-qlci merged 5 commits into
github:masterfrom
asger-semmle:jsx-factory-import
Oct 30, 2018
Merged

JS: recognize transform-react-jsx plugin#373
semmle-qlci merged 5 commits into
github:masterfrom
asger-semmle:jsx-factory-import

Conversation

@asger-semmle
Copy link
Copy Markdown
Contributor

Fixes a false positive in UnusedVariable.ql due to imports that are used by JSX, for example:

import { h } from "preact"; // Appears unused
<Foo x="y"/>

becomes

import { h } from "preact";
h(Foo, {x: "y"}); // now used here

This was already supported in some cases, but not when the name of the JSX factory came from the transform-react-jsx plugin configuration in a .babelrc file.

@asger-semmle asger-semmle requested a review from a team as a code owner October 26, 2018 11:18
Copy link
Copy Markdown

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

Basically LGTM; happy to see that it's easy to support!

/**
* Gets a file affected by this Babel configuration.
*/
Container getAContainerInScope() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We already have an appliesTo predicate in a subclass of Config; maybe we should just PUT that predicate here and improve it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hadn't noticed that, though it doesn't appear in a subclass, it appears in the other plugin class.

I've introduced a Plugin class to factor out some common logic in the two plugin classes. We now also have appliesTo on both Config and Plugin.

The getAContainerInScope() has to be there in some form or other, since the recursion steps through folders, which aren't top-levels. We can rename it if you like. I'd like to keep it public, though, so we can easily tweak it at customers.

}

/** Gets the name of the variable used to create JSX elements. */
string getJSXFactoryVariableName() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps change JSX to Jsx for consistency with the class name (and our normal camel-case conventions).

Copy link
Copy Markdown

@xiemaisi xiemaisi left a comment

Choose a reason for hiding this comment

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

One minor typo, otherwise lgtm.

this = cfg.getPluginConfig(pluginName)
}

/** Gets the name of the plugin begin installed. */
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/begin/being/?

@xiemaisi xiemaisi added the JS label Oct 30, 2018
@semmle-qlci semmle-qlci merged commit 8b866ad into github:master Oct 30, 2018
aibaars added a commit that referenced this pull request Oct 20, 2021
Ruby: warn that Ruby is still in Beta
smowton pushed a commit to smowton/codeql that referenced this pull request Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants