7$: `x += 1` reports two errors, should report one about quick-lint-js HOT 7 OPEN

I thought I linked this issue with PR #81. Apparently not! My bad.

from quick-lint-js.

This seems simple enough, I could take a swing at it.

I claim this for-hire task. I expect payment after I complete this task. I will email the quick-lint-js team if I am assigned this task.

from quick-lint-js.

@mushrom Sure, you can work on this task.

@FMMazur made some progress on this task in #81. Perhaps you can pick up where they left off.

from quick-lint-js.

Hmmmm, this is a lot more complicated than it looks, could reframe the problem a bit 👀

Current thought is that rather than having a new error type, you could (optionally) condense use errors in the output formatter, so if you have repeated use errors on the same line in the text formatter, you could output the use types and the number of errors, or something like that. This would be cleaner than adding a new use_and_assignment_... error type imo, and having a toggleable condensed/verbose mode would be a nice touch. Could further simplify things by merging the errors for assignment/use of an undeclared variable.

How does that sound, could go ahead with that if that seems fine.

from quick-lint-js.

Current thought is that rather than having a new error type, you could (optionally) condense use errors in the output formatter, so if you have repeated use errors on the same line in the text formatter, you could output the use types and the number of errors, or something like that.

I think you're only thinking of the CLI use case. The main use case of quick-lint-js is as an editor extension. In an editor, we want to highlight issues inline, and allow the programmer to see a description for each error. Grouping the errors by line doesn't make sense in general. For example, notice the squigglies under both ys on the first line of this snippet:

Your suggestion does make sense for the CLI, but I don't see how it's related to this task.

This would be cleaner than adding a new use_and_assignment_... error type imo

I think context-specific error messages are helpful for beginners. It also makes the linter feel more human and less robotic. I think a new error type makes sense.

Prior art:

• E001 & E002 & E003 & E004
  • E002 assignment to const global variable
  • E003 assignment to const variable
  • E004 assignment to const variable before its declaration
  • E001 variable assigned before its declaration
• E007 & E008
  • E007 classes cannot be named 'let'
  • E008 let statement cannot declare variables named 'let'
• E017 & E087 (though I should merge these; we only recently added a way to parameterize messages by statement_kind)
  • E017 if statement needs parentheses around condition
  • E087 while loop needs parentheses around condition
• E091 & E092
  • E091 switch statement needs parentheses around condition
  • E092 switch statement is missing '(' around condition
• E025 & E132
  • E025 missing comma between object literal entries
  • E132 missing ',' between variable declarations

Could further simplify things by merging the errors for assignment/use of an undeclared variable.

Can you explain what this would look like to the user? What would we merge the errors into? What would we print or show in their editor?

from quick-lint-js.

I think you're only thinking of the CLI use case.

Pretty much, this seems like it's mostly an issue in the CLI output. In an editor extension having two errors isn't as big a deal,
but with the CLI it quickly makes the output hard to read, so I figure the most direct way to solve that is in the CLI output formatter.

Grouping the errors by line doesn't make sense in general.

Hmmm yeah, guess this would only work for well-styled code, assuming that the code is even more than one line... yeah maybe not the best idea. Problem is then I don't see any way around tweaking the parser, adding the new traversal code and everything, much more time and effort than I wanted to put in to what looked like a simple fix.

I think context-specific error messages are helpful for beginners. It also makes the linter feel more human and less robotic. I think a new error type makes sense.

Could further simplify things by merging the errors for assignment/use of an undeclared variable.

Can you explain what this would look like to the user? What would we merge the errors into? What would we print or show in their editor?

The thing is it's a small variation on another error, the used_variable class already has a 'kind' field, why not pass that to the error formatter instead of having a seperate error? There's no loss of information there, just programmatically easier to deal with. Similar feelings on the other list of errors there, eg. missing comma, the only difference between the two is the kind (object literal entires, variable declarations) which could easily be parameterized. This was intended to be a seperate suggestion btw, would make implementing "error condensing" easier, but not necessary.

I think I'll release this for now though, I could work on merging FFMazur's code, don't feel comfortable doing that "for hire".

from quick-lint-js.

The thing is it's a small variation on another error, the used_variable class already has a 'kind' field, why not pass that to the error formatter instead of having a seperate error? There's no loss of information there, just programmatically easier to deal with.

You're right that there's no loss of information. I'm skeptical that it's easier to program. If want to provide the user with different messages, code examples, and fix suggestions depending on the 'kind' field, I'd need to cram it all into one error type.

For example, E132 (missing ',' between variable declarations) might be fixed by adding a comma, or by adding a newline (or semicolon). E025 (missing comma between object literal entries) can only be fixed by adding a comma.

As another example, E001 (variable assigned before its declaration) might be fixed by moving the declaration up above the assignment, but E004 (assignment to const variable before its declaration) can't be fixed that way. Instead, E004 needs to be fixed by making a new variable.

I think I'll release this for now though, I could work on merging FFMazur's code, don't feel comfortable doing that "for hire".

👍 You can still claim and work on this task while not accepting payment.

from quick-lint-js.