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

Type checker in IDE and CLI/CI sometimes disagree #4620

Closed
turadg opened this issue Feb 21, 2022 · 3 comments · Fixed by #9164
Closed

Type checker in IDE and CLI/CI sometimes disagree #4620

turadg opened this issue Feb 21, 2022 · 3 comments · Fixed by #9164
Assignees
Labels
bug Something isn't working tooling repo-wide infrastructure

Comments

@turadg
Copy link
Member

turadg commented Feb 21, 2022

Describe the bug

Presently, successful type checking depends on a --maxNodeModulesJsDepth flag which IDE use doesn't support. In some cases, @ts-expect-error can't be used because the IDE and CLI/CI disagree about the set of errors.

Expected behavior

Set of errors reported by type checker is consistent no matter where it's run.

Additional context

#4560 will fix that but that's a work item and this ticket is the bug itself. Easier to find by title search and more clear when linked to from code as to why. When that's resolved it would be good to find the ts-ignore we resorted to and replace them with ts-expect-error where possible. I'm assigning this to myself to take care of that.

@erights
Copy link
Member

erights commented Nov 16, 2022

Since we've made progress on removing --maxNodeModulesJsDepth, should this bug still remain open?

@turadg
Copy link
Member Author

turadg commented Nov 16, 2022

Unfortunately it's still a problem. #6575 changes the ts-ignore to ts-expect-error and there are numerous errors reported:
https://github.com/Agoric/agoric-sdk/actions/runs/3483412832/jobs/5826921492
https://github.com/Agoric/agoric-sdk/actions/runs/3483412832/jobs/5826921018

Sampling,

Error: ../vat-data/src/far-class-utils.js(206,3): error TS2322: Type 'M & RemotableBrand<{}, M>' is not assignable to type 'T & RemotableBrand<{}, T>'.
  Type 'M & RemotableBrand<{}, M>' is not assignable to type 'T'.
    'T' could be instantiated with an arbitrary type which could be unrelated to 'M & RemotableBrand<{}, M>'.
Error: ../SwingSet/src/controller/controller.js(289,5): error TS2578: Unused '@ts-expect-error' directive.
Error: ../SwingSet/src/controller/controller.js(291,5): error TS2578: Unused '@ts-expect-error' directive.
Error: ../SwingSet/src/kernel/kernelSyscall.js(208,18): error TS2532: Object is possibly 'undefined'.
Error: ../SwingSet/src/kernel/kernelSyscall.js(210,20): error TS2532: Object is possibly 'undefined'.
Error: ../SwingSet/src/kernel/state/kernelKeeper.js(217,5): error TS2578: Unused '@ts-expect-error' directive.
Error: ../SwingSet/src/lib/storageAPI.js(166,7): error TS2578: Unused '@ts-expect-error' directive.

They could probably all be solved with some additional typing work. If we did that, we still wouldn't have the "Expected behavior" ,

Set of errors reported by type checker is consistent no matter where it's run.

…because more could pop up until we solve this structurally with #4560

@turadg
Copy link
Member Author

turadg commented Jul 17, 2023

We can fix this with typescript-eslint/typescript-eslint#6754

It will probably make CI run faster too and may solve #5788

turadg added a commit that referenced this issue Mar 28, 2024
@turadg turadg mentioned this issue Mar 28, 2024
1 task
@mergify mergify bot closed this as completed in #9164 Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants