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

💅 False positive noUselessConstructor for changed data modifiers #987

Closed
1 task done
vbudovski opened this issue Dec 1, 2023 · 6 comments
Closed
1 task done
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature

Comments

@vbudovski
Copy link

Environment information

CLI:
  Version:                      1.4.1
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.10.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/8.11.0"

Biome Configuration:
  Status:                       unset

Workspace:
  Open Documents:               0

Rule name

noUselessConstructor

Playground link

https://biomejs.dev/playground/?code=YwBsAGEAcwBzACAAQgBhAHMAZQAgAHsACgAgACAAcAByAGkAdgBhAHQAZQAgAHIAZQBhAGQAbwBuAGwAeQAgAHYAYQBsADoAIABzAHQAcgBpAG4AZwA7AAoAIAAgAAoAIAAgAHAAcgBvAHQAZQBjAHQAZQBkACAAYwBvAG4AcwB0AHIAdQBjAHQAbwByACgAdgBhAGwAOgAgAHMAdAByAGkAbgBnACkAIAB7AAoAIAAgACAAIAB0AGgAaQBzAC4AdgBhAGwAIAA9ACAAdgBhAGwAOwAKACAAIAB9AAoAfQAKAAoAYwBsAGEAcwBzACAARABlAHMAYwBlAG4AZABhAG4AdAAgAGUAeAB0AGUAbgBkAHMAIABCAGEAcwBlACAAewAKACAAIABjAG8AbgBzAHQAcgB1AGMAdABvAHIAKAB2AGEAbAA6ACAAcwB0AHIAaQBuAGcAKQAgAHsACgAgACAAIAAgAHMAdQBwAGUAcgAoAHYAYQBsACkACgAgACAAfQAKAH0ACgAKAGMAbwBuAHMAdAAgAGQAIAA9ACAAbgBlAHcAIABEAGUAcwBjAGUAbgBkAGEAbgB0ACgAJwBoAGUAbABsAG8AIQAnACkACgA%3D

Expected result

The rule should not be triggered in this situation. See TypeScript playground link below for error resulting from suggested linter fix.
https://www.typescriptlang.org/play?#code/MYGwhgzhAEBCkFNoG8BQ1oAcBOBLAbmAC5LYJgAmA9gHYgCe0hIAXNBEXjQOYDc60ATiolgJCtGC0O2AK5iq2ABTM2M3DwCUKARiIALXBAB0zaAF4mYEPwwBfVA9ShIMACIIIwBDQpgaRNAIAB4kvjDwEEhoAPQxGJLSnPJEiirWapwa3Nqx8QnsspgIysyaqHEJDk5SNBzQEpY0CADu0B5ePn4BSgDk+gggIFQAhL3lQA

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Conaclos Conaclos added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug and removed S-Bug-confirmed Status: report has been confirmed as a valid bug labels Dec 1, 2023
@Conaclos
Copy link
Member

Conaclos commented Dec 1, 2023

Thanks for bringing this to light!

We currently have no way of retrieving the accessibility modifier of the parent's constructor. The only way to fix this is ignoring claases that extend a class. However I see a possible workaround: we could ignore the constructors with an explicit public accessibility modifier. TypeScript Eslint has the same issue and seems to allow this workaround (not sure that this was intentional).

TypeScript ESLint documented the caveat and decided to not fix this rare false positive.

Any opinion?

@Conaclos Conaclos added the S-Enhancement Status: Improve an existing feature label Dec 1, 2023
@vbudovski
Copy link
Author

vbudovski commented Dec 1, 2023

I can’t say I agree with the TypeScript ESLint reasoning. The reason for using TypeScript is precisely to take advantage of as much typing as possible. I don’t particularly like the explicit public specifier as a workaround, as it could cause confusion if removed. I’ve disabled the lint rule on the problematic line for now.

How difficult would it be to get access to inheritance chain?

@Conaclos
Copy link
Member

Conaclos commented Dec 1, 2023

How difficult would it be to get access to inheritance chain?

If the class is in the same file, it isp ossible. Otherwise, the infrastructure is currently uable to query other files. This requires to change the underlying infrastructure. By the way it si something we plan to do to support multi-file analysis.

@vbudovski
Copy link
Author

It’s not a huge issue for the moment. Better to wait for the proper fix with the planned multi-file analysis. Worth documenting the edge case for now. Thanks for investigating!

@Conaclos
Copy link
Member

Conaclos commented Dec 1, 2023

Definitively! We should document this.

@ematipico
Copy link
Member

This has been documented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Enhancement Status: Improve an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants