[gjs-gts-parser] fix type aware linting when using ts+gts files #1996

patricklx · 2023-11-13T14:53:26Z

this passes our own ts.sys to typescript, which emulates gts files as mts files.
We use mts extension, so that we keep the same length of import statements and so not move offsets when replacing the extension in imports.

fixes #1995

also cleaned up the code as the file was getting too big.

lib/parsers/gjs-gts-parser.js

vstefanovic97 · 2023-11-14T10:16:38Z

tests/lib/rules-preprocessor/gjs-gts-parser-test.js

+          },
+          {
+            files: ['**/*.ts'],
+            parser: '@typescript-eslint/parser',


@patricklx don't we need to leave *.ts files parsing to eslint-plugin-ember/gjs-gts-parser also?

If we don't use it then I assume we will again have errors if we try to import from a .gts file inside a regular .ts file

Though this means that we will need to recommend ppl use eslint-plugin-ember/gjs-gts-parser for all files js/ts/gjs/gts in order to have it work properly

i decided now to just overwrite the TS.sys functions, typescript-eslint already handles those cases and with this we keep it like that

aha I see, so basically we just monkey patch ts directly. Might be a bit brittle but it will work :D.
And we don't need to worry about creating our own TS program nor involving glint :D
Solid idea!

the problem i'm facing now is how to replace .gts without breaking some eslint rule...
especially the stylistic ones.
it would shift all code by 1 character... but adding something after it breaks some whitespace rules.
So now i use the extension .mts

Though this means that we will need to recommend ppl use eslint-plugin-ember/gjs-gts-parser for all files js/ts/gjs/gts in order to have it work properly

Maybe, now ts.sys is overwritten as soon as gjs-gts-parser is loaded. Which is loaded during config loading.

@patricklx yes you are right, with ts patch approach we can leave ts files to typescript-eslint/parser.
Again solid idea patching this :)

vstefanovic97 · 2023-11-14T19:54:06Z

tests/lib/rules-preprocessor/gjs-gts-parser-test.js

+    const resultErrors = results.flatMap((result) => result.messages);
+    expect(resultErrors).toHaveLength(3);
+
+    expect(resultErrors[0].message).toBe("Use 'String#startsWith' method instead."); // Actual result is "Unsafe member access [0] on an `any` value."


@patricklx We should probably remove this comment now

lib/parsers/ts-utils.js

package.json

lib/parsers/ts-utils.js

patricklx · 2023-11-23T14:27:39Z

lib/parsers/ts-utils.js

+      return content;
+    },
+  };
+  ts.setSys(newSys);


found this, now a bit better than monkey patching

tests/lib/rules-preprocessor/gjs-gts-parser-test.js

vstefanovic97 · 2023-11-24T11:43:49Z

@patricklx this seems to be working ok now as far as I can tell :D

patricklx · 2023-11-27T08:42:48Z

@bmish I squashed the commits now. this is ready

vstefanovic97 · 2023-12-06T10:41:42Z

Will we be able to merge this soon?
Ping @bmish

patricklx · 2023-12-06T10:48:53Z

rebased

bmish · 2023-12-06T16:08:59Z

lib/parsers/ts-utils.js

+
+module.exports.patchTs = function patchTs() {
+  const readDirectory = ts.sys.readDirectory;
+  ts.sys.readDirectory = function (...args) {


This appears to be overriding TypeScript functions with our own version, correct? Couldn't this affect other code using TypeScript in the user's application/repository? I don't think it's safe nor acceptable to be modifying dependencies like this.

It's only inside eslint. It should work normally as we also call the original function

that's how I understand these sorts of modifications, too

Can you find some evidence for that? I'm not convinced that's the case. TypeScript is likely installed just once in the user's repository, assuming everyone requests v5. eslint-plugin-ember doesn't necessarily have its own copy of TypeScript. So I suspect you are modifying TypeScript used by others.

Not to mention, this is not a supported API for adjusting TypeScript behavior, and presumably could break with any future version update...

All of this code is to be removed from eslint-plugin-ember to ember-eslint-parser (and then that dep added back here), and it's not the type of patch you think it is.

Happy to hop on a call if you like!

Its actually not monkey patching anymore.
Its passing our own sys to typescript via ts.setSys() if that makes any difference.
https://github.com/ember-cli/eslint-plugin-ember/pull/1996/files#diff-e244765e7358fb9a23e94c9b7d23917d4ed0172bb7550fbc1ff88359691859feR12.
It's similar to how it's done on the ts.program object in typescript-eslint

I'll be PRing the removal of all this parser code shortly, and then be collabin with Patrick to reintroduce via single dep

It looks like setSys is not public.

Anyways, I'll go ahead and release another alpha version shortly.

Here is the beginning of the extraction #2028

NullVoxPopuli · 2023-12-06T16:45:03Z

lib/parsers/transform.js

+        }
+      }
+      // block params do not have location information
+      // add our own nodes so we can reference them


why do we do anything with block params? the whole glimmer AST should not be needed for gjs/gts parsing (as far as linting is concerned)

I reorganized the files as parser file was getting to big

but that doesn't answer the question 😅

Ah, it's to have the rules no-undef and no-unused work in gjs/gts templates. Like this we have the exact location of no-undef.
And it also works for block params. Marking them unused when not used.
And js vars, that are in use in the template will not be marked as unused

You can see it in thte tests for the parser

so I guess, how much code is there just to support no-undef and no-unused having exact locations?

maybe it's no big deal -- but also maybe @glimmer/syntax should support having offsets passed to it for all the AST nodes to reduce the Math being done in eslint-plugin-ember 🤔

maybe content-tag, or another library should be responsible for stitching together the concepts of:

content-tag (split <template> from js/ts)

parse AST for JS/TS

parse AST for the <template>s

stitch all the ASTs together with correct offsets for the file

somehow have methods to allow writing back to the file for any given sub-AST range

(I see now why there is so much code involved with all of this, that is a lot of steps, and I feel like most folks overwhelmed by all this code (including me, maaaaybe @ef4 (ie: put everything in content-tag), didn't understand the full scope of the problem)

we only have this configuration for gjs/gts in template-lint: https://github.com/ember-template-lint/ember-template-lint/blob/master/lib/config/recommended.js#L98

Right, template-lint doesn't handle js scope. Here it can handle both.

It also is more code to supporting other rules that are token based

And the code could be simpler if glimmer/syntax provides ranges for block params and for the html tag parts split by .

glimmerjs/glimmer-vm#1490

I will happily take PRs on that repo that improve the parser!

NullVoxPopuli · 2023-12-06T16:52:16Z

lib/parsers/gjs-gts-parser.js

I'm a bit behind on this PR, so apologies while I ask a bunch of stupid questions 😅

I was looking at eslint-plugin-svelte, and they have something like this:

{ files: ['*.svelte'], parser: 'svelte-eslint-parser', parserOptions: { parser: { // Specify a parser for each lang. ts: '@typescript-eslint/parser', js: 'espree', typescript: '@typescript-eslint/parser' } } }

would that work for us / simplify this code?

oh, shouldn't parserOptions.programs be set somewhere? https://github.com/typescript-eslint/typescript-eslint/tree/v5.30.7/packages/parser#parseroptionsprograms

If we set a program, we also have to handle program updates... ts-eslint has a bunch of code fir that.
which is ehy i opted to pass a custom sys to ts.setSys.

makes sense, thanks!!!

well wait -- what do you mean by program updates?

@NullVoxPopuli either way it couldn't hurt to covert the code to ESM.
If you are up for it I can try to spend some time next couple of days and convert the codebase to ESM if you guys would be willing to review.
After that we can try Glint approach more easily and see if it will work? Wdyt? cc: @patricklx, @bmish

personally, I think since we're preparing for a major, now is a great time to convert to ESM -- (esp as that conversion is much easier to land than type-aware linting, which we still have lots to document and talk about (to spread knowledge, context, document issues, document with the TS team, etc))

@NullVoxPopuli I think we actually can't convert to ESM, because eslint package uses cjs so it won't be able to import parseForEslint function as well as rules if we converted them to ESM...
So I think unless glint packages start publishing cjs only approach we have is trying the dynamic import.

Yes, that's correct. I tried to make an esm plugin for eslint once and it only works with the new configuration files.

Converting to ESM may be a large effort best saved for the next major version (not the in-progress v12 release which is already packed with breaking changes) and after ESLint v9 is out.

NullVoxPopuli · 2023-12-06T17:08:02Z

lib/parsers/transform.js

Starting another unrelated thread, with some historical artefacts:

TS Member says "Vue is custom, and 'doing weird things'"
Single run detection for type-aware linting causing file not found in any of the provided program instances typescript-eslint/typescript-eslint#4093 (comment)
Leading to the exact issue we've been running in to: New vue file will get Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser. when typescript 3.9-4.0 in VSCode typescript-eslint/typescript-eslint#2127
This issue is still open even: Looking for champion to better support Vue in typescript-eslint vuejs/eslint-plugin-vue#1296
and vue-eslint-parser and typescript-eslint problems vuejs/vue-eslint-parser#104

So, given this, I am not surprised making TS work with gjs/gts is a fair bit of work. And I have a hunch this is why the Vue ecosystem has optional support for jsx/tsx, because TS built in jsx/tsx into TS parsing with no option for other syntax plugins

Here is one such way to parse custom files: https://github.com/ota-meshi/typescript-eslint-parser-for-extra-files

we could even add gjs/gts to here: https://github.com/ota-meshi/typescript-eslint-parser-for-extra-files/tree/main/src/transform

it looks like none of this matters tho, because svelte seems to have fixed it all,
their config:

/** @type { import("eslint").Linter.FlatConfig } */ module.exports = { root: true, extends: [ 'eslint:recommended', 'plugin:@typescript-eslint/recommended', 'plugin:svelte/recommended', 'prettier' ], parser: '@typescript-eslint/parser', plugins: ['@typescript-eslint'], parserOptions: { sourceType: 'module', ecmaVersion: 2020, extraFileExtensions: ['.svelte'] }, env: { browser: true, es2017: true, node: true }, overrides: [ { files: ['*.svelte'], parser: 'svelte-eslint-parser', parserOptions: { parser: '@typescript-eslint/parser' } } ] };

though, this requires generating a `.svelte-kit` folder before it can work for some reason

old comment, because they break general expectation of eslint and "just working on source files":

oh nevermind, they seem to actually be further behind: sveltejs/kit#11209

NullVoxPopuli · 2023-12-06T17:17:04Z

lib/parsers/transform.js

+const glimmer = require('@glimmer/syntax');
+const DocumentLines = require('../utils/document');
+const { visitorKeys: glimmerVisitorKeys } = require('@glimmer/syntax');
+const TypescriptScope = require('@typescript-eslint/scope-manager');


Another separate thread:

typescript-eslint/typescript-eslint#3174 (comment)

OOOOOOFFFF

NullVoxPopuli · 2023-12-08T17:33:50Z

tests/lib/rules-preprocessor/gjs-gts-parser-test.js

+              extraFileExtensions: ['.gts'],
+            },
+            extends: [
+              'plugin:@typescript-eslint/recommended-requiring-type-checking',


what's this config? I didn't see it listed here: https://typescript-eslint.io/linting/configs

It might be an old config name but it's for type-aware linting: https://typescript-eslint.io/linting/typed-linting/

'plugin:@typescript-eslint/recommended-type-checked',

vs

'plugin:@typescript-eslint/recommended-requiring-type-checking',

?

ah!
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/index.ts#L20-L22

type-checking is deprecated, but an alias for type-checked

NullVoxPopuli · 2023-12-08T17:35:00Z

tests/lib/rules-preprocessor/gjs-gts-parser-test.js

+
+    expect(resultErrors[2].line).toBe(8);
+    expect(resultErrors[2].message).toBe("Use 'String#startsWith' method instead.");
+  });
 });


can we also add a test for a failure case for a type-aware lint?

Like, any of

'@typescript-eslint/no-unsafe-argument': 'error', '@typescript-eslint/no-unsafe-assignment': 'error', '@typescript-eslint/no-unsafe-call': 'error', '@typescript-eslint/no-unsafe-member-access': 'error', '@typescript-eslint/no-unsafe-return': 'error',

NullVoxPopuli

I think these changes are looking really good, and I super appreciate the work you've all been doing on figuring this out.
(It seems that @patricklx and @vstefanovic97 are some of the first folks to figure out type-aware linting for non-js/x syntaxes!!!!)

I'm approving to move things forward, though I'd like to see a type-aware error test added before merge.

After merging, How do ya'll feel about extracting the parser to a separate package (so it can have its own tests, lots more tests, etc, and be conceptually separated from "lint configuration", which is what the spirit of eslint-plugin-ember is?)

Then, eslint-plugin-ember can bundle that extracted package as a dependencies entry, so consumers don't need to worry about installing anything separate.

I have a feeling this would put many of the lint-package maintainers at ease, as parsing and line-offset-altering is intense, and not for the faint of heart!

(It also allows us to iterate on the parser without re-releasing eslint-plugin-ember (thanks semver!))

edit: Made the repo here, and added @patricklx and @vstefanovic97 as collaborators on it
https://github.com/NullVoxPopuli/ember-eslint-parser

patricklx · 2023-12-12T09:57:04Z

@NullVoxPopuli found a way to sync the mts & gts internal source files.
also, ts is now handled optional

set our own ts.sys with ts.setSys typescript-eslint has handling for a lot of scenarios for file changes and project changes etc use mts extension to keep same offsets we also sync the mts with the gts files

patricklx · 2023-12-12T19:23:01Z

@NullVoxPopuli there are already type aware tests on the string with the [0] check. Lint needs to know that it is actually a string. Otherwise it will not show that error

NullVoxPopuli reviewed Nov 13, 2023

View reviewed changes

lib/parsers/gjs-gts-parser.js Outdated Show resolved Hide resolved

vstefanovic97 reviewed Nov 13, 2023

View reviewed changes

lib/parsers/gjs-gts-parser.js Outdated Show resolved Hide resolved

patricklx force-pushed the fix-type-aware branch from fe54a17 to a70475d Compare November 14, 2023 09:21

patricklx changed the title ~~attempt fix type aware~~ fix type aware linting with when using ts+gts files Nov 14, 2023

patricklx force-pushed the fix-type-aware branch 2 times, most recently from 5efa7b2 to 486d505 Compare November 14, 2023 09:50

vstefanovic97 reviewed Nov 14, 2023

View reviewed changes

patricklx marked this pull request as ready for review November 14, 2023 13:44

patricklx force-pushed the fix-type-aware branch from 7e496d9 to db50e78 Compare November 14, 2023 13:44

patricklx changed the title ~~fix type aware linting with when using ts+gts files~~ [gjs-gts-parser] fix type aware linting with when using ts+gts files Nov 14, 2023

vstefanovic97 reviewed Nov 14, 2023

View reviewed changes

vstefanovic97 reviewed Nov 15, 2023

View reviewed changes

lib/parsers/ts-utils.js Outdated Show resolved Hide resolved

bmish reviewed Nov 16, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

bmish added the Bug label Nov 16, 2023

vstefanovic97 reviewed Nov 22, 2023

View reviewed changes

lib/parsers/ts-utils.js Outdated Show resolved Hide resolved

patricklx commented Nov 23, 2023

View reviewed changes

patricklx force-pushed the fix-type-aware branch from 1b23108 to dd72c1c Compare November 23, 2023 14:29

vstefanovic97 reviewed Nov 23, 2023

View reviewed changes

tests/lib/rules-preprocessor/gjs-gts-parser-test.js Show resolved Hide resolved

patricklx force-pushed the fix-type-aware branch from 3e1e945 to 7feaa03 Compare November 23, 2023 16:38

patricklx changed the title ~~[gjs-gts-parser] fix type aware linting with when using ts+gts files~~ [gjs-gts-parser] fix type aware linting when using ts+gts files Nov 24, 2023

patricklx force-pushed the fix-type-aware branch from 8608c46 to 1b5d584 Compare November 27, 2023 08:42

patricklx force-pushed the fix-type-aware branch from 1b5d584 to 7268a31 Compare December 6, 2023 10:48

bmish reviewed Dec 6, 2023

View reviewed changes

NullVoxPopuli reviewed Dec 6, 2023

View reviewed changes

patricklx force-pushed the fix-type-aware branch 2 times, most recently from 6f21d2f to 35c39d3 Compare December 8, 2023 14:31

NullVoxPopuli reviewed Dec 8, 2023

View reviewed changes

NullVoxPopuli approved these changes Dec 8, 2023

View reviewed changes

NullVoxPopuli mentioned this pull request Dec 11, 2023

type-aware lints error with default generate eslint config sveltejs/kit#11209

Closed

patricklx force-pushed the fix-type-aware branch from 7393d21 to 77fe5c8 Compare December 12, 2023 09:55

patricklx force-pushed the fix-type-aware branch from 77fe5c8 to 97d7398 Compare December 12, 2023 10:01

fix gts type aware by emulating them to ts files

a93ea4b

set our own ts.sys with ts.setSys typescript-eslint has handling for a lot of scenarios for file changes and project changes etc use mts extension to keep same offsets we also sync the mts with the gts files

patricklx force-pushed the fix-type-aware branch from 97d7398 to a93ea4b Compare December 12, 2023 10:10

NullVoxPopuli merged commit 49f2690 into ember-cli:master Dec 12, 2023
8 checks passed

NullVoxPopuli mentioned this pull request Dec 12, 2023

Move gjs/gts parser to ember-eslint-parser library #2028

Merged

2 tasks

[gjs-gts-parser] fix type aware linting when using ts+gts files #1996

[gjs-gts-parser] fix type aware linting when using ts+gts files #1996

Conversation

patricklx commented Nov 13, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstefanovic97 commented Nov 24, 2023

patricklx commented Nov 27, 2023

vstefanovic97 commented Dec 6, 2023

patricklx commented Dec 6, 2023

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

bmish Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

patricklx Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

patricklx Dec 6, 2023 • 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

vstefanovic97 Dec 7, 2023 • 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

Choose a reason for hiding this comment

NullVoxPopuli Dec 6, 2023 • 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

NullVoxPopuli left a comment • edited Loading

Choose a reason for hiding this comment

patricklx commented Dec 12, 2023 • edited Loading

patricklx commented Dec 12, 2023

patricklx commented Nov 13, 2023 •

edited

Loading

bmish Dec 12, 2023 •

edited

Loading

NullVoxPopuli Dec 6, 2023 •

edited

Loading

patricklx Dec 6, 2023 •

edited

Loading

NullVoxPopuli Dec 6, 2023 •

edited

Loading

patricklx Dec 6, 2023 •

edited

Loading

patricklx Dec 6, 2023 •

edited

Loading

vstefanovic97 Dec 7, 2023 •

edited

Loading

NullVoxPopuli Dec 6, 2023 •

edited

Loading

NullVoxPopuli left a comment •

edited

Loading

patricklx commented Dec 12, 2023 •

edited

Loading