Don't count a write-only reference as a use #17752
Conversation
ghost
force-pushed
the
write-reference
branch
7 times, most recently
from
1440332 to
a548bf9
Compare
|
@sheetalkamat Any comments? |
src/compiler/checker.ts
Outdated
| const links = getNodeLinks(node); | ||
| if (!links.resolvedSymbol) { | ||
| links.resolvedSymbol = !nodeIsMissing(node) && resolveName(node, node.escapedText, SymbolFlags.Value | SymbolFlags.ExportValue, Diagnostics.Cannot_find_name_0, node, Diagnostics.Cannot_find_name_0_Did_you_mean_1) || unknownSymbol; | ||
| const isReference = !isWriteAccess(node, /*writeOnly*/ true); |
There was a problem hiding this comment.
move this to the call of resolveName in case node is missing.
src/compiler/utilities.ts
Outdated
| /** | ||
| * @param writeOnly If set, return true for `x++;` but not for `f(x++);`, which uses `x` in addition to writing to it. | ||
| */ | ||
| export function isWriteAccess(node: Node, writeOnly: boolean): boolean { |
There was a problem hiding this comment.
isWriteAccess(.., /*writeOnly*/ true) is ugly allover the place. consider splitting them into two functions, one for isWriteOnlyAccess, and one isReadAccess
|
|
|
|
ghost
added
the
| ==== tests/cases/compiler/unusedParameterUsedInTypeOf.ts (1 errors) ==== | ||
| function f1 (a: number, b: typeof a) { | ||
| ~ | ||
| !!! error TS6133: 'b' is declared but never used. |
There was a problem hiding this comment.
I think because of the intent of this test, the test aught to be modified not to trigger this diagnostic.
|
I also wounder if we should change the error message to be more specific. instead of |
ghost
force-pushed
the
write-reference
branch
from
2631beb to
2a535c6
Compare
|
@weswigham @mhegazy Good to go? |
|
👍 |
src/compiler/utilities.ts
Outdated
| /** | ||
| * @param writeOnly If set, return true for `x++;` but not for `f(x++);`, which uses `x` in addition to writing to it. | ||
| */ | ||
| function isWriteAccessWorker(node: Node, writeOnly: boolean): boolean { |
There was a problem hiding this comment.
writeOnly feels like a misnomer, since, based on its usage with isWriteOnlyAccess and isReadOnlyAccess, the parameter toggles between readonly and writeonly functionality. Maybe the function should be called accessCheckWorker and isWriteAccess?
There was a problem hiding this comment.
isReadOnlyAccess returns !isWriteAccessWorker(node, /*writeOnly*/ false); -- isWriteAccessWorker(node, false); is true for any write, so its negation is whether the access is read-only.
I've changed this to an enum-returning function so there are fewer negations and it's easier to read.
src/compiler/utilities.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * @param writeOnly If set, return true for `x++;` but not for `f(x++);`, which uses `x` in addition to writing to it. |
There was a problem hiding this comment.
I don't think this comment is supposed to be here anymore; but, this is much nicer now. Would AccessKind be better than WriteKind, considering it can indicate a read?
|
Oh, and can you get a PR updating our RWC baselines with this? Considering it found unused variables in our codebase, I'm sure it'll do the same in our RWC suite. |
|
I actually didn't see any breaks in RWC. |
|
Huh. Neat. |
3 participants
Fixes #11499
Adds an additional parameter to
resolveNamethat determines whether we would count this as a use.