Add visible alias statements for non-visible binding patterns when emitting declaration #48869

Andarist · 2022-04-28T07:23:07Z

…itting declaration

Andarist · 2022-04-28T07:23:32Z

src/compiler/checker.ts

+                            && isDeclarationVisible(declaration.parent.parent.parent.parent.parent)) {
+                            return addVisibleAlias(declaration, declaration.parent.parent.parent.parent);
+                        }
+                        else if (symbol.flags & SymbolFlags.BlockScopedVariable) {


the new code is only within this else if block

Andarist · 2022-04-28T07:30:25Z

tests/baselines/reference/declarationEmitExpressionInExtends6.errors.txt

-    const { Foo } = require("./a");
-    export default class extends Foo {}
-                                 ~~~
-!!! error TS4021: 'extends' clause of exported class has or is using private name 'Foo'.


The error is gone here as now destructured variables can be visible - so this is a good change. However, maybe this test should be rewritten somehow but I'm not sure how. How unique it was? There are already some other tests with this error reported for the extends.

This test was introduced in #39567 and later tweaked in #39631 . Maybe @a-tarasyuk could give me some insight here.

Also note that now the declaration for this test case is still not emitted - the emit output didn't change, you can see it here:
https://github.com/microsoft/TypeScript/blob/61bdfa9697f638e0eb94ca57ec4204139b9a0dbd/tests/baselines/reference/declarationEmitExpressionInExtends6.js

I'm not sure if this should be emitted or not, the mixed TS+JS situation is confusing to me and I have no expectations of my own when it comes to such a setup. No other error is raised here despite the .d.ts not being generated - which looks suspicious to me. However, I've quickly checked the same test, just without a binding pattern:

// @module: commonjs // @declaration: true // @allowJs: true // @types: node // @currentDirectory: / // @Filename: /node_modules/@types/node/index.d.ts declare const require: any; // @Filename: /a.js module.exports = class Foo {} // @Filename: /b.ts const Foo = require("./a"); export default class extends Foo {}

And it behaves the same way - so at the very least this shouldn't be treated as a regression from this PR here.

Ok, so this affected test case actually fails on the CI: https://github.com/microsoft/TypeScript/runs/6206598678?check_suite_focus=true

AssertionError: Program has no source file with name '/a.js'

I've gone to retest this on my machine and it's somewhat weird - I didn't see it when rerunning tests, this only starts to be reported AFTER I accept baselines. So when I've executed tests & just accepted baselines without rerunning stuff once again everything looked fine.

I've quickly set this situation up in a kitchen sink project and it seems that .d.ts and .js can be emitted for both a.js and b.ts just fine. Can this perhaps be an issue with the testing harness in this repo?

I've checked and those are the all file seen by the filesByName:

/b.ts /node_modules/@types/node/index.d.ts /.ts/lib.d.ts /.ts/lib.es5.d.ts /.ts/lib.dom.d.ts /.ts/lib.webworker.importscripts.d.ts /.ts/lib.scripthost.d.ts

So indeed there is no source related to a.js here 🤔

I don't really know what's going on here. If I go all the way back to #41257 and apply your change there, this still crashes. I don't really know what the deal is, how a source file could just not exist, or why this change would cause this to manifest.

Ye, this is super weird - I have a strong suspicion that this was some well-hidden bug in the testing harness because in the real project the filesByName list is OK (it includes the a.js file, IIRC).

I will get back to this in a couple of days and try to dig in more - if you have any suggestions or expectations about those involved cases I would really appreciate them. For some things I just lack context and it's harder for me to assess if the thing that I'm observing is correct or if it's a bug :p

I've been trying to figure this one out; what I can see is that before your PR, no d.ts files are generated for declarationEmitExpressionInExtends6, but after, it does. But, that d.ts file is generated for a.js, which is very weird.

Perhaps this has something to do with the fact that the code you've added is doing an unbounded findAncestor, and the code surrounding it checks (weirdly, but I don't know the code) a specific set of parents only?

But, that d.ts file is generated for a.js, which is very weird.

Hm, I think I was testing this in a real project after seeing this failure and it seems to be generate:

are you saying that it shouldn't be generated?

I'm not sure precisely, but if I go one commit before your change on your branch, the test harness doesn't hit this error because some dts.size is zero.

Ok, I dug into this deeper and I think I mostly got the situation correct.

.d.ts should be generated in this situation because this test is using allowJs: true

however, the current test setup doesn't implement things exactly in the same way as the "real thing", it works on some assumptions for this case

because of that the .js that should be treated as one of the input files is put into this.otherFiles

this wasn't a problem up until now because this test didn't emit any .d.ts because it contained a compilation error so a source file for a.js was never requested (nor for b.ts)

the compilation error was fixed by this PR so suddenly source files were started to be requested because, as you have mentioned, d.ts was started to be emitted here

I have no intention to fix the test harness here as that is out of the scope of this PR - so I've decided to rewrite this test to use import * as+ destructuring as I think that in essence, it was the same as the require-based test. It's worth highlighting that this import-based test results in the same error on the current main branch that the require-based did. The error is gone though because the underlying issue was fixed by this PR.

Thanks for looking at this harder. I agree that there seems to be something wrong with the test harness that needs to be fixed otherwise this'll trigger for unrelated reasons. Changing the test seems fine.

I'm not sure if we track bugs in the harness in actual issues, but maybe that's where we should put it as to not forget (with a reference back to this thread).

…ter the changes

jakebailey

LGTM, but I'd appreciate someone else checking that I didn't miss something here. (I was only waiting thanks to that odd harness error.)

Andarist · 2022-06-27T21:17:19Z

@jakebailey who would be the most appropriate additional reviewer for this?

jakebailey · 2022-06-28T16:50:58Z

@weswigham is listed as the other reviewer; he's definitely an appropriate reviewer.

jakebailey · 2022-06-28T16:52:49Z

src/compiler/checker.ts

+                            return addVisibleAlias(declaration, declaration.parent.parent.parent.parent);
+                        }
+                        else if (symbol.flags & SymbolFlags.BlockScopedVariable) {
+                            const variableStatement = findAncestor(declaration, isVariableStatement)!;


My only remaining comment is the new branch's use of findAncestor, as compared to the existing code's direct use of parents and specific checks; I'm not entirely certain that it matters but I do wonder how similar the two paths in the new isBindingElement block are and if they should look the same and/or be sharing code.

I think it makes sense here because binding patterns can be nested and thus I can't use specific checks like the other branches. The PR has tests covering those "recursive" scenarios.

I see, thanks.

Actually, does this need to have some negative termination condition to prevent it from going too far, e.g. not go out to the wrong scope? Or is that impossible?

I don't think that it's possible to go out too far here - I'm targeting the nearest variable statement that is guaranteed to be in the current scope. It's also not possible (I think) that this process statements from inner scopes as if it did then we'd have bugs in other branches of this code too.

jakebailey · 2022-07-06T17:08:12Z

Well, I'm confident enough that I'll just merge this. Thanks for working on this and waiting.

Add visible alias statements for non-visible binding patterns when em…

61bdfa9

…itting declaration

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 28, 2022

Andarist commented Apr 28, 2022

View reviewed changes

typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 28, 2022

sandersn requested review from jakebailey and weswigham May 9, 2022 21:26

sandersn assigned sandersn and jakebailey and unassigned sandersn May 9, 2022

Andarist added 2 commits June 20, 2022 09:18

Merge branch 'main' into fix/dts-obj-pattern-visibility

133f346

Rewrite declarationEmitExpressionInExtends6 test to make it pass af…

6ce7697

…ter the changes

jakebailey approved these changes Jun 20, 2022

View reviewed changes

jakebailey reviewed Jun 28, 2022

View reviewed changes

jakebailey merged commit 8687940 into microsoft:main Jul 6, 2022

Andarist deleted the fix/dts-obj-pattern-visibility branch July 6, 2022 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add visible alias statements for non-visible binding patterns when emitting declaration #48869

Add visible alias statements for non-visible binding patterns when emitting declaration #48869

Andarist commented Apr 28, 2022 •

edited

Loading

Andarist Apr 28, 2022

Andarist Apr 28, 2022

Andarist Apr 28, 2022 •

edited

Loading

jakebailey May 26, 2022

Andarist May 27, 2022

jakebailey Jun 7, 2022

Andarist Jun 7, 2022

jakebailey Jun 7, 2022

Andarist Jun 20, 2022

jakebailey Jun 20, 2022

jakebailey left a comment

Andarist commented Jun 27, 2022

jakebailey commented Jun 28, 2022

jakebailey Jun 28, 2022

Andarist Jun 29, 2022

jakebailey Jun 29, 2022

jakebailey Jun 29, 2022

Andarist Jun 30, 2022

jakebailey commented Jul 6, 2022

Add visible alias statements for non-visible binding patterns when emitting declaration #48869

Add visible alias statements for non-visible binding patterns when emitting declaration #48869

Conversation

Andarist commented Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey left a comment

Choose a reason for hiding this comment

Andarist commented Jun 27, 2022

jakebailey commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented Jul 6, 2022

Andarist commented Apr 28, 2022 •

edited

Loading

Andarist Apr 28, 2022 •

edited

Loading