fix: do not match MySymbol.import("mod") by joscha · Pull Request #23358 · microsoft/TypeScript

joscha · 2018-04-12T09:34:40Z

The quill library has a static method called import on it's exported symbol, which is used like this:

Quill.import('dependency');

These dependencies are not actually imports, they are strings in a map. Unfortunately ts.preProcessFile thinks they are valid imports.
I tried fixing the issue, but there is some logic missing and I am unsure how to debug the test I added properly with me being able to look at the scanner state. Maybe someone can give me a hint on how to complete my fix, please?

There is an associated issue that is labeled
'Bug' or 'help wanted' or is in the Community milestone
Code is up-to-date with the master branch
You've successfully run jake runtests locally [But the code fails, and I am unsure why]
You've signed the CLA
There are new or updated unit tests validating the change

Fixes #23359

msftclas · 2018-04-12T09:34:52Z

All CLA requirements met.

joscha · 2018-04-12T09:37:48Z

                        pos++;
                        return token = SyntaxKind.MinusToken;
                    case CharacterCodes.dot:
+                        tokenFlags |= TokenFlags.PrecedingDot;


this might need to move right before the return if we understand the flag to denote a single dot.

mhegazy · 2018-04-12T21:59:04Z

        function tryConsumeImport(): boolean {
            let token = scanner.getToken();
-            if (token === SyntaxKind.ImportKeyword) {
+            if (token === SyntaxKind.ImportKeyword && !scanner.hasPrecedingDot()) {


do not think you need to change the scanner API to do this. this whole function is just a mini parser. it advances the scanner each time it needs to consume a token, so you can do it here, no need to change the scanner implementation. for instance, you could have a flag for lastTokenWasDot in nextToken

I was trying to avoid putting state into the parser, as I saw we had https://github.com/Microsoft/TypeScript/blob/7631ad03a330511418f5cad421b9b876002fcca3/src/compiler/types.ts#L1578-L1579 already. If you are happy for me to store that in a local var, then I will adapt the pull request?

that is not the parser.. this is just a small parser-like function that tries to speculatively find import/export/require statements.

Okay, sorry for my wording, I used parser, because you called it a mini parser. So flag in nextToken SGTY?

yeah. it is already keeping state, one more flag should not harm anyone.

Ok, will give it a spin 👍

changes in ee429ef, but for some reason it doesn't work as expected. I am not even sure if the code is run, as I can't figure out from the docs on how to debug the tests or use the watch mode. Plus, process.stdout.write and console.log are not available either, so I am unsure on how to debug this - any pointers @mhegazy?

jake runtests-browser should run the tests int he browser, you can use the browser dev tools to debug.

mhegazy · 2018-04-13T16:27:34Z

            else if (token === SyntaxKind.CloseBraceToken) {
                braceNesting--;
            }
+            lastTokenWasDot = token === SyntaxKind.DotToken;


should not this be before we consume the next token? this just says token === SyntaxKind.DotToken

Of course, fixed up in 02b8160

mhegazy · 2018-04-13T16:29:01Z

For debugging, I just use jake runtests-browser and use the browser devtools to debug. you can also write a node module that imports typescript.js and calls preprocessfile on it.

joscha · 2018-04-14T15:23:56Z

For debugging, I just use jake runtests-browser and use the browser devtools to debug. you can also write a node module that imports typescript.js and calls preprocessfile on it.

That works like a charm, thank you!

joscha · 2018-04-14T15:24:43Z

~~Tests should go ✅ hopefully, PTAL @mhegazy~~

joscha · 2018-04-14T16:02:52Z

I don't really understand, the test passes locally:

joscha · 2018-04-14T16:35:24Z

OK, got it, no hot-reload, need to restart after file-changes:

joscha · 2018-04-14T17:35:03Z

didn't need to introduce more state after all @mhegazy - tests pass locally after a fresh build via gulp clean && gulp runtests-browser --browser=chrome --tests=non-imports, hopefully CI will back me up this time 🤞 😬

mhegazy · 2018-04-14T19:53:24Z

        let ambientExternalModules: { ref: FileReference, depth: number }[];
        let braceNesting = 0;
-        let lastTokenWasDot = false;
+        let lastToken: SyntaxKind = null;


do not use null. undefined (or unintialized) is what we have used throughout the code base.

Yep, I saw, the linter yelled at me :)

mhegazy · 2018-04-14T19:55:32Z

+                if (token === SyntaxKind.EndOfFileToken) {
                    break;
                }
+                if (token === SyntaxKind.DotToken) {


what about a.b.import?

Should still work, because the next token is a literal, which means that it will go into the else came - I will add a test!

ah, I see what you mean now. Will fix.

mhegazy · 2018-04-16T18:33:11Z

        function nextToken() {
-            const token = scanner.scan();
-            if (token === SyntaxKind.OpenBraceToken) {
+            if (currentToken) {


do not think you need this check.

mhegazy · 2018-04-16T19:38:09Z

thanks @joscha!

fix: do not match MySymbol.import("mod")

4da2e5e

joscha commented Apr 12, 2018

View reviewed changes

joscha mentioned this pull request Apr 12, 2018

Code Quill.import('mod'); makes TS think it needs to import('mod') #23359

Closed

mhegazy reviewed Apr 12, 2018

View reviewed changes

use state

ee429ef

mhegazy reviewed Apr 13, 2018

View reviewed changes

store lastToken

02b8160

undefined instead of null

f4fa2eb

fix up

c64c1c9

mhegazy reviewed Apr 14, 2018

View reviewed changes

joscha added 2 commits April 15, 2018 11:27

detect nested non-imports

4dc9db1

revert change

a3311ed

mhegazy reviewed Apr 16, 2018

View reviewed changes

Remove condition

f0057f9

mhegazy merged commit a8618a7 into microsoft:master Apr 16, 2018

joscha deleted the joscha/fix-non-imports-matching branch April 16, 2018 21:47

microsoft locked and limited conversation to collaborators Jul 31, 2018

Conversation

joscha commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Apr 13, 2018

Uh oh!

joscha commented Apr 14, 2018

Uh oh!

joscha commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joscha commented Apr 14, 2018

Uh oh!

joscha commented Apr 14, 2018

Uh oh!

joscha commented Apr 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joscha commented Apr 12, 2018 •

edited

Loading

msftclas commented Apr 12, 2018 •

edited

Loading

mhegazy Apr 12, 2018 •

edited

Loading

joscha commented Apr 14, 2018 •

edited

Loading