Isolated declarations errors #58201
Isolated declarations errors #58201
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
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. |
| export function createSyntacticTypeNodeBuilder(options: CompilerOptions) { | ||
| const strictNullChecks = getStrictOptionValue(options, "strictNullChecks"); | ||
|
|
||
| return { |
There was a problem hiding this comment.
These will probably benefit from caching their result - declaration emit is actually invoked twice right now, once for the diagnostics before emit, and once later for the actual emit (the perils of declaration emit being blocked on the presence of errors). Keeping a cache of nodes for which the result has already been computed should save some time, since it's basically guaranteed every node will have these called twice on them (and they only need to report the diagnostic the first time around).
There was a problem hiding this comment.
I have two questions with regard to this:
-
The true/false result is less important, the errors are generated through calling
reportInferenceFallbackon the actual node that causes the issue. Should this be caching the nodes (as there could be multiple) that caused the fallback to inference? -
Currently we return true/false but the final end state is that this is integrated into the
NodeBuilderin the checker and that it will actually generate new type nodes instead. I don't think caching the synthetic type nodes is something the currentNodeBuilderdoes. Am I wrong about this?
There was a problem hiding this comment.
The true/false result is less important, the errors are generated through calling reportInferenceFallback on the actual node that causes the issue. Should this be caching the nodes (as there could be multiple) that caused the fallback to inference?
You could, but probably don't need to. If you have a cached result, the diagnostics should have already been emitted, so you'll just issue duplicates that'll get deduped anyway.
I don't think caching the synthetic type nodes is something the current NodeBuilder does. Am I wrong about this?
Yes, actually. typeParameterToName has caching, since the same parameter often gets serialized many times, though you're right that there's no top-level cache.
| @@ -43,7 +43,7 @@ exitCode:: ExitStatus.Success | |||
|
|
|||
|
|
|||
| //// [/src/project/class1.d.ts] | |||
| declare const a = 1; | |||
| declare const a: MagicNumber; | |||
There was a problem hiding this comment.
How did this end up changing? I think it's probably a good change, but it does seem odd.
There was a problem hiding this comment.
In shouldPrintWithInitializer I now test specifically if a type annotation already exists.
function shouldPrintWithInitializer(node: Node): node is CanHaveLiteralInitializer & { initializer: Expression; } {
return canHaveLiteralInitializer(node)
&& !node.type // <- New test
&& !!node.initializer
&& resolver.isLiteralConstDeclaration(getParseTreeNode(node) as CanHaveLiteralInitializer); // TODO: Make safea
}There was a problem hiding this comment.
Whatever made us ignore the type annotation here in the first place is odd. I can only get it to repro with the same global-type-alias-referencing-a-default-expr-type structure as is in this test.
There was a problem hiding this comment.
I did look into it a very long time ago. I was something about the freshness of the literal type. But if the type annotation is there then there is no reason to look further and it is more in line with what an external tool would need to do as well.
There was a problem hiding this comment.
We're probably failing to widen (and strip literal freshness) from export default expr types. Or the typeof type operation of them does, at least. Unsure. It's probably a bug.
There was a problem hiding this comment.
IIRC we still haven't solved the problem of freshness in general when it comes to isolatedDeclarations, either...
There was a problem hiding this comment.
In what sense? ensureNoInitializer will emit an error if the initializer is not a primitive. It is true that if you add the annotation, the meaning will not be exactly the same though.
const Original = 1;
const Alias = Original; // adding :1 here will chnage the type of v
let v = Alias; // v is number if Alias is not annotated, or 1 if const Alias:1
tests/baselines/reference/isolatedDeclarationErrorsClassesExpressions.errors.txt
Show resolved
Hide resolved
…isolated declarations command line option.
There was a problem hiding this comment.
Largely this seems OK, but there are two larger classes of things to move - a lot of the context.addDiagnostic calls being made in the transform don't seem to depend on any emit-related information - all that can should be moved forward into the checker, probably (generally, we prefer being able to emit diagnostics without emitting actual output wherever possible - removing the existing cases in declaration emit that issue errors late is a long-standing backlog item). isolatedModules works that way already, anyway. Additionally, a lot of the new declaration diagnostic selection logic should get moved to src\compiler\transformers\declarations\diagnostics.ts, since that's where all the other declaration emit diagnostic selection helpers already are. Lastly, there are some other small issues not related to those two larger themes. I've mostly avoided nits for expressionToTypeNode.ts, since I know it's where a lot of follow-up changes are occurring.
| syntacticNodeBuilder.serializeTypeOfExpression(expr, context, addUndefined); | ||
| } | ||
| context.flags |= NodeBuilderFlags.NoSyntacticPrinter; | ||
| const result = expressionOrTypeToTypeNode(context, expr, type, addUndefined); |
There was a problem hiding this comment.
| const result = expressionOrTypeToTypeNode(context, expr, type, addUndefined); | |
| const result = expressionOrTypeToTypeNodeHelper(context, expr, type, addUndefined); | |
(the function referenced here should be renamed) and the containing function should be renamed to expressionOrTypeToTypeNode. As-is, this is still only being invoked at that withContext callsite (which should also be expressionOrTypeToTypeNode once this is renamed), so that NoSyntacticPrinter flag isn't even doing anything.
There was a problem hiding this comment.
expressionOrTypeToTypeNodeHelper (after rename) may call typeToTypeNodeHelper which in turn will call serializeTypeForDeclaration and serializeReturnTypeForSignature and these would perform the check again. I can refactor those into serializeTypeForDeclarationHelper and serializeReturnTypeForSignatureHelper which do not perform the check and serializeTypeForDeclaration and serializeReturnTypeForSignature which would only be called from the top level. This would ensure the code does not check multiple times the same expression, and would remove the need for the NoSyntacticPrinter flag.
The flag will disappear in the next PRs. It's definitely not something that will remain. When the syntactic builder actually builds the types and falls back to the checked builder when it can't the problem of duplicate checks will go away.
Should I do the serializeTypeForDeclarationHelper and serializeReturnTypeForSignatureHelper refactor for now? Or ca we live with this until after the beta?
tests/baselines/reference/isolatedDeclarationErrorsObjects.errors.txt
Outdated
Show resolved
Hide resolved
dragomirtitian
force-pushed
the
isolated-declarations-errors
branch
from
349e65a to
0b83d8a
Compare
Co-authored-by: Sheetal Nandi <[email protected]>
dragomirtitian
force-pushed
the
isolated-declarations-errors
branch
from
f184551 to
5f3721c
Compare
|
@typescript-bot test it |
|
@weswigham Here are the results of running the user tests comparing Everything looks good! |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top 400 repos comparing Everything looks good! |
…ns-errors-fix Move methods off of context, fix context bug
|
I left some unused code behind, sorry :( Shouldn't affect benchmarks, so @typescript-bot perf test this |
|
Starting jobs; this comment will be updated as builds start and complete.
|
…ns-errors-fix-2 Fix unused code lints
…ns-errors-fix-3 Make isEntityNameVisible have the same parameter order
|
@typescript-bot perf test this faster |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added command line flag for isolated declarations that will report errors on constructs that can't easily be inferred from local information.
Some examples (not an exhaustive list):