Uh oh!
There was an error while loading. Please reload this page.
Refactored node builder flags and tests - #59440
Conversation
TypeScript Bot (typescript-bot)
commented
Jul 27, 2024
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up. |
Armando Aguirre (armanio123)
commented
Jul 27, 2024
TypeScript Bot (@typescript-bot) test this |
TypeScript Bot (typescript-bot)
commented
Jul 27, 2024
Armando Aguirre (@armanio123) Here are the results of running the user tests with tsc comparing Everything looks good! |
TypeScript Bot (typescript-bot)
commented
Jul 27, 2024
Hey Armando Aguirre (@armanio123), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
Jul 27, 2024
Armando Aguirre (@armanio123) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Jul 27, 2024
Armando Aguirre (@armanio123) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Jake Bailey (jakebailey)
commented
Jul 29, 2024
#59282 gives back a node flag, can we not just wait for that? |
Uh oh!
There was an error while loading. Please reload this page.
Armando Aguirre (armanio123)
commented
Jul 29, 2024
I was planning on adding a new flag to it, that's how I noticed this issue. So even if we reduce the number of flags, we're going to be dealing with this limitation. |
Uh oh!
There was an error while loading. Please reload this page.
76de2ca to
3185381CompareArmando Aguirre (armanio123)
commented
Aug 3, 2024
Refactored as suggested during the design meeting. There's a couple of tests that looks suspicious to me so I appreciate extra set of eyes on them. |
Jake Bailey (jakebailey)
commented
Aug 3, 2024
Yeah, I suspect there's a bug lurking in there; most of the baseline changes are unexpected. |
Uh oh!
There was an error while loading. Please reload this page.
Jake Bailey (jakebailey)
commented
Aug 3, 2024
Skimming the code locally, though, I can't find the place where things may have gone wrong... It all seems correct to me. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -222,7 +223,7 @@ export function addNewNodeForMemberSymbol( | |||
| case SyntaxKind.PropertyDeclaration: | |||
| let flags = NodeBuilderFlags.NoTruncation; | |||
There was a problem hiding this comment.
This doesn't need AllowUnresolvedNames like the code below?
Sort of wondering if all old users of NoTruncation need that flag, but maybe we shouldn't add ones where we don't think it should matter except those where we would have set it manually...
There was a problem hiding this comment.
Currently is not needed to pass the test cases. Thought we might be missing some, because adding it doesn't change any of them.
There was a problem hiding this comment.
Right, our test suite is just not always exhaustive, so I suspect that there are some lurking bugs without this flag for these auto-import / codefix cases, given those seem to in other cases depend on this flag
| contextNode, | ||
| scriptTarget, | ||
| NodeBuilderFlags.NoTruncation, | ||
| /*internalFlags*/ undefined, |
There was a problem hiding this comment.
This makes me wonder if we are missing a test with an unresolved name for those who call this code. (Specifically fixAddMissingMember)
There was a problem hiding this comment.
I agree, that's the same thing I just responded to your last comment.
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
Overall this now seems good, though the title needs to be updated to not say it's an experiment, and I do think we may need to be adding the unresolved name thing in a couple of places that maybe we just don't have tests for?
I think that the tests that fail without the change to createSignatureDeclarationFromSignature may need to be recreated as tests which make the other cases where NoTruncation failed also fail without AllowUnresolvedNames.
Armando Aguirre (armanio123)
commented
Aug 6, 2024
If I understand the interfaceA{foo: import("./doesntexist.js").Foo;getbar(): import("./doesntexist.js").Foo;}classBimplementsA{|<--Triggercompletionhere}Completion returns: foo: any;// This should have been `foo: import("./doesntexist.js").Foo;`getbar(): any {// and this `get bar(): import("./doesntexist.js").Foo;`thrownewError("Method not implemented.");} |
Jake Bailey (jakebailey)
commented
Aug 7, 2024
I would expect those to not spit out |
That's correct, we spit out |
Jake Bailey (jakebailey)
commented
Aug 7, 2024
If you don't want to write a test for those other branches, can we just put that flag back on the places that used to have |
Armando Aguirre (armanio123)
commented
Aug 8, 2024
Yeah, I was going to propose the same as I don't know how to create all of the necessary tests. I'm going to put the flag in everyplace I find |
| function typeToString(type: Type, enclosingDeclaration?: Node, flags: TypeFormatFlags = TypeFormatFlags.AllowUniqueESSymbolType | TypeFormatFlags.UseAliasDefinedOutsideCurrentScope, writer: EmitTextWriter = createTextWriter("")): string { | ||
| const noTruncation = compilerOptions.noErrorTruncation || flags & TypeFormatFlags.NoTruncation; | ||
| const typeNode = nodeBuilder.typeToTypeNode(type, enclosingDeclaration, toNodeBuilderFlags(flags) | NodeBuilderFlags.IgnoreErrors | (noTruncation ? NodeBuilderFlags.NoTruncation : 0)); | ||
| const typeNode = nodeBuilder.typeToTypeNode(type, enclosingDeclaration, toNodeBuilderFlags(flags) | NodeBuilderFlags.IgnoreErrors | (noTruncation ? NodeBuilderFlags.NoTruncation : NodeBuilderFlags.None), noTruncation ? InternalNodeBuilderFlags.AllowUnresolvedNames : InternalNodeBuilderFlags.None); |
There was a problem hiding this comment.
This scenario seems weird to me as it depends on NoTruncation being set in TypeFormatFlags. Nonetheless, this is the current behavior, so I decided to change it but let me know if you disagree and I can rollback.
There was a problem hiding this comment.
Hm, I think it's a little weird, though I guess it's also true that nobody would have ever intentionally set AllowUnresolvedNames here because that was never a valid TypeFormatFlag.
Sort of leaning towards not doing this one and seeing what happens? I assume no test changes here either.
There was a problem hiding this comment.
They don't change. Actually, the latest commit doesn't change any of the tests. I'll rollback this line.
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
I think this seems alright now, though it would be nice if we could figure out the rest.
Given we just added a flag everywhere, curious what Wesley Wigham (@weswigham) thinks, but it should at least maintain our old behavior which I feel is pretty safe.
The
NodeBuilderFlagsenum has shifted more than the allowed signed int32 limit.This refactors experiments with creating a type and constant usingThis approach was discarded becuase of the performance of BigInt, and the limitations the type has.BigIntinstead.Added an additional flag
InternalNodeBuilderFlags, that releases a couple of bits onNodeBuilderFlagby moving all the internal flags.Reference to this PR #58475.