Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[quotemark] Excuse more backtick edge cases #4642

Merged
merged 5 commits into from Apr 16, 2019
Merged

[quotemark] Excuse more backtick edge cases #4642

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95899d6
[quotemark] Excuse more backtick edge cases
ericbf Apr 5, 2019
765a634
[quotemark] Add tests for different versions
ericbf Apr 5, 2019
6f94a59
Merge branch 'master' into fix-quotemark-edge-cases
adidahiya Apr 16, 2019
4aa87c1
Update quotemarkRule.ts
adidahiya Apr 16, 2019
886a8b1
Update quotemarkRule.ts
adidahiya Apr 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 95 additions & 6 deletions src/rules/quotemarkRule.ts
View file
Open in desktop
Expand Up @@ -134,13 +134,14 @@ function walk(ctx: Lint.WalkContext<Options>) {
(isExportDeclaration(node.parent) ||
// This captures `import blah from "package"`
isImportDeclaration(node.parent) ||
// This captures kebab-case property names in object literals (only when the node is not at the end of the parent node)
(node.parent.kind === ts.SyntaxKind.PropertyAssignment &&
node.end !== node.parent.end) ||
// This captures the kebab-case property names in type definitions
node.parent.kind === ts.SyntaxKind.PropertySignature ||
// This captures quoted names in object literal keys
isNameInAssignment(node) ||
// This captures quoted signatures (property or method)
isSignature(node) ||
// This captures literal types in generic type constraints
node.parent.parent.kind === ts.SyntaxKind.TypeReference)
isTypeConstraint(node) ||
// Whether this is the type in a typeof check with older tsc
isTypeCheckWithOldTsc(node))
) {
return;
}
Expand Down Expand Up @@ -245,3 +246,91 @@ function getJSXQuotemarkPreference(
// If the regular pref is backtick, use double quotes instead.
return regularQuotemarkPreference !== "`" ? regularQuotemarkPreference : '"';
}

/**
* Whether this node is a type constraint in a generic type.
* @param node The node to check
* @return Whether this node is a type constraint
*/
function isTypeConstraint(node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral) {
let parent = node.parent.parent;

// If this node doesn't have a grandparent, it's not a type constraint
if (parent == undefined) {
return false;
}

// Iterate through all levels of union, intersection, or parethesized types
while (
parent.kind === ts.SyntaxKind.UnionType ||
parent.kind === ts.SyntaxKind.IntersectionType ||
parent.kind === ts.SyntaxKind.ParenthesizedType
) {
parent = parent.parent;
}

return (
// If the next level is a type reference, the node is a type constraint
parent.kind === ts.SyntaxKind.TypeReference ||
// If the next level is a type parameter, the node is a type constraint
parent.kind === ts.SyntaxKind.TypeParameter
);
}

/**
* Whether this node is the signature of a property or method in a type.
* @param node The node to check
* @return Whether this node is a property/method signature.
*/
function isSignature(node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral) {
let parent = node.parent;

if (ts.version < "2.7.1" && node.parent.kind === ts.SyntaxKind.LastTypeNode) {
// In older versions, there's a "LastTypeNode" here
parent = parent.parent;
}

return (
// This captures the kebab-case property names in type definitions
parent.kind === ts.SyntaxKind.PropertySignature ||
// This captures the kebab-case method names in type definitions
parent.kind === ts.SyntaxKind.MethodSignature
);
}

/**
* Whether this node is the method or property name in an assignment/declaration.
* @param node The node to check
* @return Whether this node is the name in an assignment/decleration.
*/
function isNameInAssignment(node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral) {
if (
node.parent.kind !== ts.SyntaxKind.PropertyAssignment &&
node.parent.kind !== ts.SyntaxKind.MethodDeclaration
) {
// If the node is neither a property assignment or method declaration, it's not a name in an assignment
return false;
}

return (
// In typescript below 2.7.1, don't change values either
ts.version < "2.7.1" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly checking ts.version is a little unusual... maybe there's a cleaner way? Thoughts @adidahiya?

Also, @ericbf, could you elaborate on why this is necessary?

Copy link
Contributor Author

@ericbf ericbf Apr 7, 2019 •
edited

Choose a reason for hiding this comment

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

Typescript below the listed version fails compilation (or otherwise misbehaves) when backticks are used in some places where it succeeds in future versions. For instance, typeof str === `string`​ does not narrow the type of str in TS versions below 2.7.1, causing compilation errors if you try to use it as a string later. In the other cases, older TS throws errors where NoSubstitutionLiterals are used instead of single/double quoted strings, but works fine in above 2.7.1; rather than cripple the rule to support order versions, I split it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the typeof issue here. Its milestone was 2.7 (hence the split in this PR).
And here's the issue for backticks strings as types. This one was 2.6, but rather than doing multiple splits, I just did one at 2.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created corresponding issue, #4645

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! This LGTM but before merging I'd like to hear from adidahiya on whether there's a cleaner way to check on these things.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it's unusual and should be avoided if possible, but this seems like a reasonable use case for checking ts.version.

However, can you please use the semver package and the helper function from parse.ts to check version ranges instead?

import { lt } from "semver";
import { getNormalizedTypescriptVersion } from "../../verify/parse";

function hasOldTscBacktickBehavior() {
    return lt(getNormalizedTypescriptVersion(), "2.7.1");
}

// If this node is not at the end of the parent
node.end !== node.parent.end
);
}

function isTypeCheckWithOldTsc(node: ts.StringLiteral | ts.NoSubstitutionTemplateLiteral) {
if (ts.version >= "2.7.1") {
// This one only affects older typescript versions (below 2.7.1)
return false;
}

if (node.parent.kind !== ts.SyntaxKind.BinaryExpression) {
// If this isn't in a binary expression
return false;
}

// If this node has a sibling that is a TypeOf
return node.parent.getChildren().some(n => n.kind === ts.SyntaxKind.TypeOfExpression);
}
30 changes: 18 additions & 12 deletions test/rules/quotemark/backtick/test.ts.fix
View file
Open in desktop
Expand Up @@ -6,21 +6,27 @@ var single = `single`;
var singleWithinDouble = `'singleWithinDouble'`;
var doubleWithinSingle = `"doubleWithinSingle"`;
var tabNewlineWithinSingle = `tab\tNewline\nWithinSingle`;

var array: Array<"literal string"> = [];
var arrayTwo: Array<"literal string" | number> = [];
var arrayThree: Array<"literal string" | "hello world"> = [];
var arrayFour: Array<"literal string" | "hello world" | "foo bar"> = [];
var array: Array<"literal string"> = [];
var arrayTwo: Array<"literal string" & number> = [];
var arrayFour: Array<"literal string" | "hello world" & "foo bar"> = [];

function test<T extends "generic">() {

}

function test<T extends ("generic" & number)>() {

}

const callback = <U extends "generic">() => `hi` as number | string

var hello: `world`;
`escaped'quotemark`;

// "avoid-template" option is not set.
`foo`;

const object: {
"hello-kebab"
: number
"kebab-case": number
"another-kebab": `hello-value`
} = {
"hello-kebab"
: 4
"kebab-case": 3,
"another-kebab": `hello-value`
};
49 changes: 28 additions & 21 deletions test/rules/quotemark/backtick/test.ts.lint
View file
Open in desktop
Expand Up @@ -2,34 +2,41 @@ import { Something } from "some-package"
export { SomethingElse } from "another-package"

var single = 'single';
~~~~~~~~ [' should be `]
~~~~~~~~ [single]
var double = "married";
~~~~~~~~~ [" should be `]
~~~~~~~~~ [double]
var singleWithinDouble = "'singleWithinDouble'";
~~~~~~~~~~~~~~~~~~~~~~ [" should be `]
~~~~~~~~~~~~~~~~~~~~~~ [double]
var doubleWithinSingle = '"doubleWithinSingle"';
~~~~~~~~~~~~~~~~~~~~~~ [' should be `]
~~~~~~~~~~~~~~~~~~~~~~ [single]
var tabNewlineWithinSingle = 'tab\tNewline\nWithinSingle';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [' should be `]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [single]

var array: Array<"literal string"> = [];
var arrayTwo: Array<"literal string" | number> = [];
var arrayThree: Array<"literal string" | "hello world"> = [];
var arrayFour: Array<"literal string" | "hello world" | "foo bar"> = [];
var array: Array<"literal string"> = [];
var arrayTwo: Array<"literal string" & number> = [];
var arrayFour: Array<"literal string" | "hello world" & "foo bar"> = [];

function test<T extends "generic">() {

}

function test<T extends ("generic" & number)>() {

}

const callback = <U extends "generic">() => "hi" as number | string
~~~~ [double]

var hello: "world";
~~~~~~~ [" should be `]
~~~~~~~ [double]
'escaped\'quotemark';
~~~~~~~~~~~~~~~~~~~~ [' should be `]
~~~~~~~~~~~~~~~~~~~~ [single]

// "avoid-template" option is not set.
`foo`;

const object: {
"hello-kebab"
: number
"kebab-case": number
"another-kebab": "hello-value"
~~~~~~~~~~~~~ [" should be `]
} = {
"hello-kebab"
: 4
"kebab-case": 3,
"another-kebab": "hello-value"
~~~~~~~~~~~~~ [" should be `]
};
[single]: ' should be `
[double]: " should be `
11 changes: 11 additions & 0 deletions test/rules/quotemark/backtick/test<2.7.1.ts.fix
View file
Open in desktop
@@ -0,0 +1,11 @@
if (typeof v === "string") {}

if (typeof `string` === 'number') {}

const object: {
"optional-prop"?: "hello-optional"
"another-kebab": "hello-value"
} = {
"optional-prop": undefined,
"another-kebab": "hello-value"
};
15 changes: 15 additions & 0 deletions test/rules/quotemark/backtick/test<2.7.1.ts.lint
View file
Open in desktop
@@ -0,0 +1,15 @@
[typescript]: <2.7.1
if (typeof v === "string") {}

if (typeof "string" === 'number') {}
~~~~~~~~ [double]

const object: {
"optional-prop"?: "hello-optional"
"another-kebab": "hello-value"
} = {
"optional-prop": undefined,
"another-kebab": "hello-value"
};
[single]: ' should be `
[double]: " should be `
13 changes: 13 additions & 0 deletions test/rules/quotemark/backtick/test>=2.7.1.ts.fix
View file
Open in desktop
@@ -0,0 +1,13 @@
if (typeof v === `string`) {}

if (typeof `string` === `number`) {}

const object: {
"optional-prop"?: `hello-optional`
"optional-function"?(): void
"another-kebab": `hello-value`
} = {
"optional-prop": undefined,
"optional-function"() {},
"another-kebab": `hello-value`
};
21 changes: 21 additions & 0 deletions test/rules/quotemark/backtick/test>=2.7.1.ts.lint
View file
Open in desktop
@@ -0,0 +1,21 @@
[typescript]: >=2.7.1
if (typeof v === "string") {}
~~~~~~~~ [double]

if (typeof "string" === 'number') {}
~~~~~~~~ [double]
~~~~~~~~ [single]

const object: {
"optional-prop"?: `hello-optional`
"optional-function"?(): void
"another-kebab": "hello-value"
~~~~~~~~~~~~~ [double]
} = {
"optional-prop": undefined,
"optional-function"() {},
"another-kebab": "hello-value"
~~~~~~~~~~~~~ [double]
};
[single]: ' should be `
[double]: " should be `