Skip to content
This repository was archived by the owner on Sep 16, 2021. It is now read-only.

Commit 06338d1

Browse files
TypeScript TeamKeen Yee Liau
TypeScript Team
authored and
Keen Yee Liau
committed
Add an optional file path filter to AbsoluteMatcher.
PiperOrigin-RevId: 298920299
1 parent 537bcde commit 06338d1

File tree

2 files changed

+279
-9
lines changed

2 files changed

+279
-9
lines changed

‎internal/tsetse/tests/ban_conformance_pattern/util_test.ts

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,196 @@ describe('The constant-ness logic', () => {
9595
}
9696
});
9797
});
98+
99+
describe('test AbsoluteMatcher with file path', () => {
100+
it('matched path', () => {
101+
102+
const config = {
103+
errorMessage: 'banned name with file path',
104+
kind: PatternKind.BANNED_NAME,
105+
values: ['./file_0|Foo.bar']
106+
};
107+
const sources = [
108+
`export class Foo { static bar(s: string) {return s + "abc";} }`,
109+
`import {Foo} from './file_0';
110+
var a = Foo.bar("123");`
111+
];
112+
const results =
113+
compileAndCheck(new ConformancePatternRule(config), ...sources);
114+
115+
expect(results).toHaveFailuresMatching(
116+
{matchedCode: `bar`, messageText: 'banned name with file path'});
117+
});
118+
119+
it('unmatched path', () => {
120+
const config = {
121+
errorMessage: 'banned name with file path',
122+
kind: PatternKind.BANNED_NAME,
123+
values: ['./file_0|Foo.bar']
124+
};
125+
const sources = [
126+
`export class Foo { static bar(s: string) {return s + "abc";} }`,
127+
`export class Foo { static bar(s: string) {return s + "abc";} }`,
128+
`import {Foo} from './file_1';
129+
var a = Foo.bar("123");`
130+
];
131+
132+
const results =
133+
compileAndCheck(new ConformancePatternRule(config), ...sources);
134+
expect(results).toHaveNoFailures();
135+
});
136+
137+
it('local exported definition', () => {
138+
// This is a match because Foo.bar is an exported symbol.
139+
const config = {
140+
errorMessage: 'banned name with file path',
141+
kind: PatternKind.BANNED_NAME,
142+
values: ['./file_0|Foo.bar']
143+
};
144+
const sources =
145+
[`export class Foo { static bar(s: string) {return s + "abc";} }
146+
var a = Foo.bar("123");`];
147+
148+
const results =
149+
compileAndCheck(new ConformancePatternRule(config), ...sources);
150+
expect(results).toHaveFailuresMatching(
151+
{matchedCode: `bar`, messageText: 'banned name with file path'});
152+
});
153+
154+
it('local non-exported definition', () => {
155+
// This is not a match because Foo.bar is a non-exported locally defined
156+
// symbol.
157+
const config = {
158+
errorMessage: 'banned name with file path',
159+
kind: PatternKind.BANNED_NAME,
160+
values: ['./file_0|Foo.bar']
161+
};
162+
const sources = [`class Foo { static bar(s: string) {return s + "abc";} }
163+
var a = Foo.bar("123");`];
164+
const results =
165+
compileAndCheck(new ConformancePatternRule(config), ...sources);
166+
expect(results).toHaveNoFailures();
167+
});
168+
169+
it('property test 1', () => {
170+
const config = {
171+
errorMessage: 'banned name with file path',
172+
kind: PatternKind.BANNED_NAME,
173+
values: ['./file_0|Foo.s']
174+
};
175+
const sources = [
176+
`export class Foo { static s : string; }`,
177+
`import {Foo} from './file_0';
178+
var a = Foo.s;`,
179+
];
180+
181+
const results =
182+
compileAndCheck(new ConformancePatternRule(config), ...sources);
183+
expect(results).toHaveFailuresMatching(
184+
{matchedCode: `s`, messageText: 'banned name with file path'});
185+
});
186+
187+
it('property test 2', () => {
188+
// This is a match because Moo inherits s from Foo.
189+
const config = {
190+
errorMessage: 'banned name with file path',
191+
kind: PatternKind.BANNED_NAME,
192+
values: ['./file_0|Foo.s']
193+
};
194+
const sources = [
195+
`export class Foo { static s : string; }`,
196+
`import {Foo} from './file_0';
197+
export class Moo extends Foo { static t : string; }`,
198+
`import {Moo} from './file_1';
199+
var a = Moo.s;`,
200+
];
201+
202+
const results =
203+
compileAndCheck(new ConformancePatternRule(config), ...sources);
204+
expect(results).toHaveFailuresMatching(
205+
{matchedCode: `s`, messageText: 'banned name with file path'});
206+
});
207+
208+
it('property test 3', () => {
209+
// This is not a match because Moo redefines s.
210+
const config = {
211+
errorMessage: 'banned name with file path',
212+
kind: PatternKind.BANNED_NAME,
213+
values: ['./file_0|Foo.s']
214+
};
215+
const sources = [
216+
`export class Foo { static s : string; }`,
217+
`import {Foo} from './file_0';
218+
export class Moo extends Foo { static s : string; }`,
219+
`import {Moo} from './file_1';
220+
var a = Moo.s;`,
221+
];
222+
223+
const results =
224+
compileAndCheck(new ConformancePatternRule(config), ...sources);
225+
expect(results).toHaveNoFailures();
226+
});
227+
228+
it('inheritance test 1', () => {
229+
// This is a match because Moo inherits bar from Foo.
230+
const config = {
231+
errorMessage: 'banned name with file path',
232+
kind: PatternKind.BANNED_NAME,
233+
values: ['./file_0|Foo.bar']
234+
};
235+
const sources = [
236+
`export class Foo { static bar(s: string) {return s + "abc";} }`,
237+
`import {Foo} from './file_0';
238+
export class Moo extends Foo { static far(s: string) {return s + "def";} }`,
239+
`import {Moo} from './file_1';
240+
Moo.bar("abc");`
241+
];
242+
243+
const results =
244+
compileAndCheck(new ConformancePatternRule(config), ...sources);
245+
expect(results).toHaveFailuresMatching(
246+
{matchedCode: `bar`, messageText: 'banned name with file path'});
247+
});
248+
249+
it('inheritance test 2', () => {
250+
// This is not a match because Moo redefines bar.
251+
const config = {
252+
errorMessage: 'banned name with file path',
253+
kind: PatternKind.BANNED_NAME,
254+
values: ['./file_0|Foo.bar']
255+
};
256+
const sources = [
257+
`export class Foo { static bar(s: string) {return s + "abc";} }
258+
export class Moo extends Foo { static bar(s: string) {return s + "def";} }`,
259+
`import {Foo, Moo} from './file_0';
260+
Moo.bar("abc");`
261+
];
262+
263+
const results =
264+
compileAndCheck(new ConformancePatternRule(config), ...sources);
265+
expect(results).toHaveNoFailures();
266+
});
267+
268+
it('interface', () => {
269+
// This is not a match because even though bar specified is interface Moo,
270+
// its actual definition is in class Boo.
271+
const config = {
272+
errorMessage: 'banned name with file path',
273+
kind: PatternKind.BANNED_NAME,
274+
values: ['./file_1|Moo.bar']
275+
};
276+
const sources = [
277+
`export class Foo { static bar(s: string) {return s + "abc";} }`,
278+
`import {Foo} from './file_0';
279+
export interface Moo extends Foo { }`,
280+
`import {Moo} from './file_1';
281+
export class Boo implements Moo { static bar(s: string) {return s + "def";} }`,
282+
`import {Boo} from './file_2';
283+
Boo.bar("abc");`,
284+
];
285+
286+
const results =
287+
compileAndCheck(new ConformancePatternRule(config), ...sources);
288+
expect(results).toHaveNoFailures();
289+
});
290+
});

‎internal/tsetse/util/match_symbol.ts

Lines changed: 86 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import * as ts from 'typescript';
22
import {dealias, debugLog, isAmbientDeclaration, isDeclaration, isInStockLibraries, isPartOfImportStatement} from './ast_tools';
33

4+
const PATH_NAME_FORMAT = '[/\\.\\w\\d_-]+';
45
const JS_IDENTIFIER_FORMAT = '[\\w\\d_-]+';
56
const FQN_FORMAT = `(${JS_IDENTIFIER_FORMAT}\.)*${JS_IDENTIFIER_FORMAT}`;
67
// A fqn made out of a dot-separated chain of JS identifiers.
7-
const ABSOLUTE_RE = new RegExp(`^${FQN_FORMAT}$`);
8+
const ABSOLUTE_RE = new RegExp(`^(${PATH_NAME_FORMAT}\\|){0,1}${FQN_FORMAT}$`);
89

910
/**
1011
* This class matches symbols given a "foo.bar.baz" name, where none of the
@@ -14,18 +15,59 @@ const ABSOLUTE_RE = new RegExp(`^${FQN_FORMAT}$`);
1415
* strongly suggest finding the expected symbol in externs to find the object
1516
* name on which the symbol was initially defined.
1617
*
17-
* TODO(rjamet): add a file-based optional filter, since FQNs tell you where
18-
* your imported symbols were initially defined. That would let us be more
19-
* specific in matches (say, you want to ban the fromLiteral in foo.ts but not
20-
* the one from bar.ts).
18+
* This matcher supports an optional file path filter. The filter specifies
19+
* (part of) the path of the file in which the symbol of interest is defined.
20+
* To add a file path filer to name "foo.bar.baz", prepend the path and
21+
* the separator "|" to the name. For example, "path/to/file.ts|foo.bar.baz".
22+
* With this filter, only symbols named "foo.bar.baz" that are defined in a path
23+
* that contains "path/to/file.ts" are matched.
24+
*
25+
* This filter is useful when mutiple symbols have the same name but
26+
* you want to match with a specific one. For example, assume that there are
27+
* two classes named "Foo" defined in /path/to/file0 and /path/to/file1.
28+
* // in /path/to/file0
29+
* export class Foo { static bar() {return "Foo.bar in file0";} }
30+
*
31+
* // in /path/to/file1
32+
* export class Foo { static bar() {return "Foo.bar in file1";} }
33+
*
34+
* Suppose that these two classes are referenced in two other files.
35+
* // in /path/to/file2
36+
* import {Foo} from /path/to/file0;
37+
* Foo.bar();
38+
*
39+
* // in /path/to/file3
40+
* import {Foo} from /path/to/file1;
41+
* Foo.bar();
42+
*
43+
* An absolute matcher "Foo.bar" without a file filter will match with both
44+
* references to "Foo.bar" in /path/to/file2 and /path/to/file3.
45+
* An absolute matcher "/path/to/file1|Foo.bar", however, only matches with the
46+
* "Foo.bar()" in /path/to/file3 because that references the "Foo.bar" defined
47+
* in /path/to/file1.
48+
*
49+
* Note that an absolute matcher will match with any reference to the symbol
50+
* defined in the file(s) specified by the file filter. For example, assume that
51+
* Foo from file1 is extended in file4.
52+
*
53+
* // in /path/to/file4
54+
* import {Foo} from /path/to/file1;
55+
* class Moo { static tar() {return "Moo.tar in file4";} }
56+
* Moo.bar();
57+
*
58+
* An absolute matcher "/path/to/file1|Foo.bar" matches with "Moo.bar()" because
59+
* "bar" is defined as part of Foo in /path/to/file1.
2160
*/
2261
export class AbsoluteMatcher {
2362
/**
24-
* From a "path/to/file.ts:foo.bar.baz" or "foo.bar.baz" matcher
63+
* From a "path/to/file.ts|foo.bar.baz" or "foo.bar.baz" matcher
2564
* specification, builds a Matcher.
2665
*/
27-
constructor(readonly bannedName: string) {
28-
if (!bannedName.match(ABSOLUTE_RE)) {
66+
readonly filePath: string;
67+
readonly bannedName: string;
68+
69+
constructor(spec: string) {
70+
if (!spec.match(ABSOLUTE_RE)) {
2971
throw new Error('Malformed matcher selector.');
3072
}
3173

@@ -35,12 +77,21 @@ export class AbsoluteMatcher {
3577
// on `foo`. To avoid any confusion, throw there if we see `prototype` in
3678
// the spec: that way, it's obvious that you're not trying to match
3779
// properties.
38-
if (this.bannedName.match('.prototype.')) {
80+
if (spec.match('.prototype.')) {
3981
throw new Error(
4082
'Your pattern includes a .prototype, but the AbsoluteMatcher is ' +
4183
'meant for non-object matches. Use the PropertyMatcher instead, or ' +
4284
'the Property-based PatternKinds.');
4385
}
86+
87+
if (spec.includes('|')) {
88+
// File path is present so we split spec by the separator "|".
89+
[this.filePath, this.bannedName] = spec.split('|', 2);
90+
} else {
91+
// File path is omitted so spec is bannedName.
92+
this.filePath = '';
93+
this.bannedName = spec;
94+
}
4495
}
4596

4697
matches(n: ts.Node, tc: ts.TypeChecker): boolean {
@@ -79,6 +130,14 @@ export class AbsoluteMatcher {
79130
// either a declare somewhere, or one of the core libraries that
80131
// are loaded by default.
81132
if (!fqn.startsWith('"')) {
133+
// If this matcher includes a file path, it means that the targeted symbol
134+
// is defined and explicitly exported in some file. If the current symbol
135+
// is not associated with a specific file (because it is a local symbol or
136+
// ambient symbol), it is not a match.
137+
if (this.filePath !== '') {
138+
return false;
139+
}
140+
82141
// We need to trace things back, so get declarations of the symbol.
83142
const declarations = s.getDeclarations();
84143
if (!declarations) {
@@ -91,6 +150,24 @@ export class AbsoluteMatcher {
91150
return false;
92151
}
93152
}
153+
// If we know the file info of the symbol, and this matcher includes a file
154+
// path, we check if they match.
155+
else {
156+
if (this.filePath !== '') {
157+
const last = fqn.indexOf('"', 1);
158+
if (last === -1) {
159+
throw new Error('Malformed fully-qualified name.');
160+
}
161+
const sympath = fqn.substring(1, last);
162+
debugLog(`The file path of the symbol is ${sympath}`);
163+
if (!sympath.match(this.filePath)) {
164+
debugLog(
165+
`The file path of the symbol does not match the ` +
166+
`file path of the matcher`);
167+
return false;
168+
}
169+
}
170+
}
94171

95172
debugLog(`all clear, report finding`);
96173
return true;

0 commit comments

Comments
 (0)