-
Notifications
You must be signed in to change notification settings - Fork 176
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
Improve S1874 (deprecation
): Report deprecations from TypeScript compiler
#4122
Conversation
4e8a805
to
17d2136
Compare
SonarQube Quality Gate |
|
||
import defaultImport, {deprecatedFunction, anotherDeprecatedFunction as aliasForDeprecated, notDeprecated1, notDeprecated2} from './cb.fixture.deprecations'; | ||
import defaultImport, {deprecatedFunction, anotherDeprecatedFunction as aliasForDeprecated, notDeprecated1, notDeprecated2} from './cb.fixture.deprecations'; // Noncompliant 2 |
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.
TypeScript reports on imported deprecated symbols, which we didn't previously.
@@ -81,7 +81,7 @@ let deprecatedVar; | |||
const const1 = 1, | |||
const2 = 2; | |||
|
|||
h(const1 + const2); // Noncompliant | |||
h(const1 + const2); // Noncompliant 2 |
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.
We had a false negative before on const2
.
@@ -93,7 +93,7 @@ function fn() { } | |||
|
|||
fn<number>(); | |||
fn(1); // Noncompliant | |||
h(fn); // Noncompliant | |||
h(fn); |
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.
TypeScript doesn't report deprecated symbols used as references, like here with fn
.
/* i4008 */ | ||
|
||
/** @deprecated */ function someDeprecated(a: string): string; | ||
/** @param a */ function someDeprecated(a: number): number; | ||
function someDeprecated(a: number | string): number | string { | ||
if (typeof a === 'number') return a + 1; | ||
return a; | ||
} | ||
|
||
someDeprecated('yolo'); // Noncompliant | ||
someDeprecated(42); // OK | ||
someDeprecated; // OK | ||
|
||
/** @deprecated */ function allDeprecated(a: string): string; | ||
/** @deprecated */ function allDeprecated(a: number): number; | ||
function allDeprecated(a: number | string): number | string { | ||
if (typeof a === 'number') return a + 1; | ||
return a; | ||
} | ||
|
||
allDeprecated('yolo'); // Noncompliant | ||
allDeprecated(42); // Noncompliant | ||
allDeprecated; // OK |
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.
Taken from #4064
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.
Really good job!
Fixes #4008
Building from #4064, it turns out quite tricky to cover every single case of deprecated symbols that are used. In fact, after looking at TypeScript's codebase, there are many places where usages of deprecated APIs are detected and reported. This is due to several edge cases. Instead of replicating the logic for every single one, we propose to extract and propagate deprecation suggestions emitted by TypeScript's type checker. There are minor functional differences compared to our current implementation, like the fact that TypeScript reports on import statements. However, with these changes, we behave precisely like TypeScript compilers, thus reducing false positives and negatives.