Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add no-undef #304

Merged
merged 35 commits into from
Sep 12, 2020
Merged

Add no-undef #304

merged 35 commits into from
Sep 12, 2020

Conversation

kdy1
Copy link
Contributor

@kdy1 kdy1 commented Sep 8, 2020

I confused no-global-assign with no-undef.
I thought no-global was about prohibiting assignment to global variables.
So I implemented it, but the name of such pass was no-undef.

@kdy1 kdy1 marked this pull request as ready for review September 8, 2020 08:44
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I'll check that it works as expected on deno codebase before landing.

src/rules/no_undef.rs Outdated Show resolved Hide resolved
src/rules/no_undef.rs Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

bartlomieju commented Sep 8, 2020

Some false positives:

    await new Promise((resolve: () => void, _) => {
      setTimeout(resolve, 100);
    });

// 
(no-undef) resolve is not defined
      setTimeout(resolve, 100);
                 ^^^^^^^
    at /Users/biwanczuk/dev/deno/cli/tests/unit/net_test.ts:331:17
const checkErr = (e: Error): void => {
      if (e.message === "Listener has been closed") {
        assertEquals(acceptErrCount, 1);
      } else if (e.message === "Another accept task is ongoing") {
        acceptErrCount++;
      } else {
        throw new Error("Unexpected error message");
      }
    };

// 
(no-undef) e is not defined
    const checkErr = (e: Error): void => {
                      ^^^^^^^^
    at /Users/biwanczuk/dev/deno/cli/tests/unit/net_test.ts:136:22

(no-undef) e is not defined
      if (e.message === "Listener has been closed") {
          ^
    at /Users/biwanczuk/dev/deno/cli/tests/unit/net_test.ts:137:10

Other builtins that should be listed:

  • Symbol
  • TextEncoder
  • TextDecoder
  • undefined
  • Deno
  • Uint8Array
  • Error

Edit: Actually there's way more globals to list (https://www.npmjs.com/package/globals) - @kdy1 I'll update that list later, when you're done with the logic.

@kdy1 kdy1 marked this pull request as draft September 8, 2020 09:57
@kdy1
Copy link
Contributor Author

kdy1 commented Sep 8, 2020

False positives were caused by the bug in ts_resolver, which is now fixed.

@kdy1 kdy1 mentioned this pull request Sep 9, 2020
@bartlomieju
Copy link
Member

@kdy1 some false positives I found

Class constructor param:

(no-undef) options is not defined
  constructor(options: ContextOptions) {
              ^^^^^^^^^^^^^^^^^^^^^^^
    at /Users/biwanczuk/dev/deno/std/wasi/snapshot_preview1.ts:305:14

(no-undef) options is not defined
    this.args = options.args ? options.args : [];
                ^^^^^^^
    at /Users/biwanczuk/dev/deno/std/wasi/snapshot_preview1.ts:306:16

Case using enum variant:

(no-undef) OpCode is not defined
        case OpCode.Close: {
             ^^^^^^
    at /Users/biwanczuk/dev/deno/std/ws/mod.ts:262:13

Class name inside class method:

(no-undef) Buffer is not defined
  value: Buffer,
         ^^^^^^
    at /Users/biwanczuk/dev/deno/std/node/buffer.ts:592:9

Catch binding:

(no-undef) err is not defined
  } catch (err) {
           ^^^
    at /Users/biwanczuk/dev/deno/std/ws/example_client.ts:56:11

@bartlomieju
Copy link
Member

@kdy1 the only missing globals I found are Headers and TransformStream

@kdy1
Copy link
Contributor Author

kdy1 commented Sep 9, 2020

Got it, thanks!

@kdy1
Copy link
Contributor Author

kdy1 commented Sep 11, 2020

Published a new version of swc_ecma_visit, swc_ecma_transforms, swc_ecmascript.

Patch of swc_ecma_visit is about reducing compile time while debugging, and patch of swc_ecma_transforms is about fixing resolver.

I copied some failing test cases from deno std_lint and verified locally that new version of resolver make the lint work properly.

@bartlomieju
Copy link
Member

@kdy1 thanks, I will start upgrades soon

@kdy1 kdy1 marked this pull request as ready for review September 12, 2020 09:36
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@bartlomieju bartlomieju merged commit dd82c26 into denoland:master Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants