Perform usage analysis on refenced binding aliases in function signatures. #55683
Conversation
typescript-bot
added
the
For Uncommitted Bug
dragomirtitian
force-pushed
the
fix-55654
branch
from
e8729f8 to
aad9692
Compare
|
Maybe I missed it somewhere, but I feel like I still haven’t heard an explanation as to why #55654 is worth spending 1500 lines and extra CPU cycles to fix and not just a harmless aesthetic quirk. |
|
The focus of this is |
|
@andrewbranch Most of the 1.5k are tests 😅. The actual implementation is about 350 lines, most of which is just a switch to call the appropriate node factory to recreate the node. As for CPU cycles, that was one of my worries too. As far as I can tell the only extra CPU cycles that are spent in the common case of not using the type alias in the signature is a call to While it's not a design goal to 100% reproduce TS declaration files with an external emitter, it would be ideal to limit cases where the two differ. This would be one example of a difference taht we can eliminate without. Always keeping the alias in is also a way to achieve this. |
|
I guess I’m surprised that the more complicated behavior is the one we’re pursuing if the goal is consistency between tsc and a third-party |
|
@andrewbranch Most of the machinery to track references is needed for tracking the usage of variables and imports anyway. This extra analysis for binding elements shouldn't make a third-party emitter much more complicated IMO. Someone obviously cared enough to implement the removal of aliases, even if it did have some bugs, fixing the bugs seems the better way forward. They are just unused implementation details that should probably not be exposed unless absolutely necessary. |
… keyword properties)
typescript-bot
added
For Backlog Bug
|
This PR theoretically looks good to me, though the repetition in the d.ts emitter makes me feel like there must be a better way still that I'm missing here. Maybe @weswigham or @rbuckton know a better trick. |
That is already an error: |
|
So, as far as I can tell, this looks good to me, though I'm not a super fan of the repetition I don't think there's much of a way to do better. @weswigham do you have any other feedback here? I'm hoping to get the list of differences for isolatedDeclarations down to as small as possible (I think think this is the only one left??) |
There was a problem hiding this comment.
Hmm, this looks fine to me, implementation-wise. I think I'd have a minor preference for merging the nodes from bindingElementToMakeVisible and aliasesToMakeVisible into a single nodesToMakeVisible list, but it's not strictly required if nobody else feels strongly about it.
As in, make it |
Aye. |
|
I think the awkward thing is that there's only one |
I could merge those, the problem is that will change the common case to something like: if (symbolAccessibilityResult.aliasesToMakeVisible) {
if(!lateMarkedStatements) {
lateMarkedStatements = []
}
for (const ref of symbolAccessibilityResult.aliasesToMakeVisible) {
if(ref.kind === SyntaxKind.BindingElement) {
const bindingElement = ref;
const parameter = findAncestor(bindingElement, isParameter);
Debug.assert(parameter !== undefined);
const parent = getOriginalNode(parameter.parent);
let aliases = usedBindingElementAliases.get(parent);
if (!aliases) {
usedBindingElementAliases.set(parent, aliases = new Map());
}
aliases.set(getOriginalNode(bindingElement), bindingElement.name);
} else {
pushIfUnique(lateMarkedStatements, ref);
}
}
}Which would mean some extra work filtering out the binding elements first time
@jakebailey I don't think there is any way to have multiple binding elements to make visible if the code is valid. You can get it if with duplicate identifiers but I don't think that is a case we need to think to deeply about as it's a syntax error ( |
|
I think I'd personally prefer to leave it as is, in that case. I don't feel strongly, but it does seem like an extra loop to find one thing... |
|
I'll quick run the extended suite, though I expect it to not really do anything since it can't detect emit changes. @typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at ca275ec. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at ca275ec. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at ca275ec. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at ca275ec. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
…ures. (microsoft#55683) Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
|
Just to note it, this is being reverted in #57020. |
Fix #55654 by marking binding aliases that need to be preserved.
This is done in a two step process:
Fixes #55654