(incomplete) new failing test, <a target="_blank noopener">

[mcast]

I'm offering this pull request as a sort of feature request: that this lint rule could detect when correct code was clearly intended but it failed.

It is going to need some discussion before merging! What would you like to do?

(I confess that I have not run the new code, I don't currently have a dev environment for it and this PR is fallout from an internal code review. I'm trying to balance between what this project might need and what my work projects need ⚖️)

---

this test is derived from a real bug in a project,
in which the intention was to get it right
but the bug causes "two wrongs make a right"

this one failing test is intended to illustrate an entire class of possible failures
which are neither detected nor thoroughly represented

and although the apparent intention of the "bad" code is
target="_blank" the effect is not, so unless the browser DWIMs it
then the jsx-no-target-blank rule is technically not violated

[mcast]

Linking an example of where the new failure happens
at https://github.com/jsx-eslint/eslint-plugin-react/actions/runs/3226022289/jobs/5279041693#step:5:85
(secondary bug: #step:5:85 does not jump to the relevant line in my browser)

  1) jsx-no-target-blank
       invalid
         <a href="https://example.com/7a-run-on" target="_blank rel=noopener"></a>
// features: [], parser: default:

      AssertionError [ERR_ASSERTION]: Should have 1 error but had 0: []
      + expected - actual

      -0
      +1
      
      at testInvalidTemplate (node_modules/eslint/lib/rule-tester/rule-tester.js:815:24)
      at Context.<anonymous> (node_modules/eslint/lib/rule-tester/rule-tester.js:1039:29)
      at process.processImmediate (node:internal/timers:471:21)

That is the new test failure, and it occurs many times across versions.

[ljharb]

I'm a bit confused. I'm not sure how we could reliably infer that in the general case, even if we could in a specific case.

In other words, I don't think this is possible for a computer to "clearly" determine, altho it may be so for a human.

[mcast]

Thanks for the quick reply.

I'm a bit confused. I'm not sure how we could reliably infer that in the general case, even if we could in a specific case.

I am also not sure what is correct, but seeing the bug pass this linter in a live project caused me to think some action is needed.

Perhaps a series of questions would help?

1. are we (for some value of we) generally agreed that the ommission of " " in the new sample is a bug?
   • The project owner fixed the bug when I pointed it out
2. are we generally agreed that this linter is in pole position to detect that bug?
   • The project owner had already configured eslint-plugin-react in the CI, and pointed me towards this rule
3. are you (as project maintainers) willing to extend the functionality to try to meet such bugs?

If "no" then there is nothing more to be done. 😃

In other words, I don't think this is possible for a computer to "clearly" determine, altho it may be so for a human.

I agree that "clearly" is going to be difficult to reach. Sometimes programmers write something which looks odd, and there is a good reason.

However this is what compiler warnings are for.

1. does eslint-plugin-react support warnings, or is it interested in giving its errors an integer "level of severity"?
   • my impression is that errors are generated for some detected problems, and a "pass" requires no errors.
2. does it have access to "ignore that rules on this tag" comments? I know these things are an ugly mark on any code, but they can be useful.
3. are there some linter configuration flags which can change the sensitivity?
   • I see the advice for .eslintrc
   • The project I reviewed uses eslint-config-airbnb, it has a .eslintignore file and I see comments like /* eslint-disable no-param-reassign */

(Sorry, I don't have much React experience yet.)

Again if "no" then there is nothing more to be done. 😃

Otherwise, in this case some heuristics might be helpful... and they might carry over to other rules. The simplest I can think of is "does target contain _blank without being equal to it?", and that does seem like a warning sign.

var attrib = '_blank nearly';

function nearly(subject, findText) {
  if (subject == findText) {
    return false;
  } else if (subject.includes(findText)) {
    return true;
  } else {
    return false;
  }
}

console.log('nearly? ' + nearly(attrib, '_blank'));

To extend nearly for edit distance at this point would seem like overkill...

Any use?

[ljharb]

are we (for some value of we) generally agreed that the ommission of " " in the new sample is a bug?

yes, of course!

are we generally agreed that this linter is in pole position to detect that bug?

i'm not sure what "pole position" is, but the linter is often in a position where it can only kind of sometimes detect the bug, which effectively means the linter shouldn't try to.

The project owner had already configured eslint-plugin-react in the CI, and pointed me towards this rule
are you (as project maintainers) willing to extend the functionality to try to meet such bugs?

If it can be done such that there's no false positives, and false negatives are an intuitive category, of course!

eslint itself - and thus all eslint plugins - only support one level of severity "stop the build, or don't" and consumers can choose, in the latter case, between "tell me about it but don't stop the build" or "ignore it". (iow, "error", and "warn" or "off").

The linter is not a compiler - JavaScript does not HAVE a compile step, even if you're transpiling - so I don't think your analogy holds.

I agree that if you can produce a warning that is never wrong, but may not be exhaustive, that it could still be useful. I'm just trying to understand if that's what this scenario is.

[ljharb]

I think i could see a general rule for "on an html element, is there an attribute value that has an = sign in it where the LHS of the = sign is a valid attribute" - which would cover this case. that would share some knowledge with the no-unknown-property rule, and would likely be able to offer suggestions but NOT autofix. Is that something you're interested in building?

[mcast]

i'm not sure what "pole position" is,

Sorry, I picked an unhelpful phrase. Pole position suggests a competition among tools, to spot this bug. I think eslist-plugin-react is the first tool outside React itself, to see the bug. The other tool I can see in this project is https://jestjs.io/ but I believe that is running hand-crafted unit tests for specific things - and I don't see it looking at this broken element.

That would make this linter the only tool with a chance to see the bug. That fits with my observing the bug only as the "context" part of a unified diff.

but the linter is often in a position where it can only kind of sometimes detect the bug, which effectively means the linter shouldn't try to.

Yes, we are now into false positives & false negatives.

If it can be done such that there's no false positives, and false negatives are an intuitive category, of course!

I tend to assume it's a "best effort" approach to exposing bugs. Linters in my experience, either out of the box or as locally configured by other users, have had... opinions that not all the users share.

This is why the "oi linter, ignore this problem in this block" comments get applied. We call the result a clean build, but it's clean because we painted over all the places where some ugly was poking through.

eslint itself - and thus all eslint plugins - only support one level of severity
[...]
The linter is not a compiler - JavaScript does not HAVE a compile step, even if you're transpiling - so I don't think your analogy holds.

I'm just looking for ways that a process which might incorrectly throw up a problem can be silenced. "Warnings" as a smaller form of "error" are a long-standing approach - some users choose to ignore them manually, others choose to promote them to errors & stop the build, and there is space in between.

While jsx-eslint has only one level of problem, it's not a useful way to reduce the possible disruption from a less-certain check.

I agree that if you can produce a warning that is never wrong, but may not be exhaustive, that it could still be useful. I'm just trying to understand if that's what this scenario is.

Sure.

I suspect it is, but as you say there may be more than one approach. I think any of them might have false positives.

---

I think i could see a general rule for "on an html element, is there an attribute value that has an = sign in it where the LHS of the = sign is a valid attribute" - which would cover this case.

That's another approach, it looks nice and may uncover a related class of bugs; it would have its own class of false positives.

that would share some knowledge with the no-unknown-property rule,

I see https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unknown-property.md

and would likely be able to offer suggestions but NOT autofix.

This class of bug is going to be hard to autofix - or not with much certainty of getting it right.

Is that something you're interested in building?

I suspect this is the end of the PR, sorry...

I found the original issue during $work, but an improvement to jsx-eslint won't be coming out of $work time - you're currently getting a timeslice of my lunch breaks, and that isn't enough to make progress. I don't see any sign that current $work is interested in supporting this kind of thing. 😞