Refactored node builder flags and tests #59440
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
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 @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
@typescript-bot test this |
|
@armanio123 Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @armanio123, the results of running the DT tests are ready. Everything looks the same! |
|
@armanio123 Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@armanio123 Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
#59282 gives back a node flag, can we not just wait for that? |
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. |
armanio123
force-pushed
the
RefactorNodeBuilderFlags
branch
from
76de2ca
to
3185381
Compare
|
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. |
|
Yeah, I suspect there's a bug lurking in there; most of the baseline changes are unexpected. |
|
Skimming the code locally, though, I can't find the place where things may have gone wrong... It all seems correct to me. |
tests/cases/fourslash/completionsClassMemberImportTypeNodeParameter2.ts
Outdated
Show resolved
Hide resolved
| @@ -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
src/services/codefixes/helpers.ts
Outdated
| @@ -506,6 +507,7 @@ export function createSignatureDeclarationFromCallExpression( | |||
| 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.
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.
armanio123
changed the title
|
If I understand the interface A {
foo: import("./doesntexist.js").Foo;
get bar(): import("./doesntexist.js").Foo;
}
class B implements A {
| <-- Trigger completion here
}Completion returns: foo: any; // This should have been `foo: import("./doesntexist.js").Foo;`
get bar(): any { // and this `get bar(): import("./doesntexist.js").Foo;`
throw new Error("Method not implemented.");
} |
|
I would expect those to not spit out |
|
That's correct, we spit out |
|
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 |
|
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 |
| @@ -5962,7 +5964,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
|
|
|||
| 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.
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 @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.