feat: warn on unknown warning codes in runes mode

[Rich-Harris]

Alternative to #11495. WIP

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

⚠️ No Changeset found

Latest commit: c7c8d49

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[dummdidumm]

I personally prefer the other approach of telling the people inline "hey this did work in legacy mode but now it no longer does". Now when converting to runes they would get two warnings and you have to connect them manually in your head (to know that they "belong" together).
I also prefer the other silencing approach with using the global context approach we're already using for warnings in other areas.
The idea of determining runes mode at the point of ignore extraction is nice.

[dummdidumm]

I could be persuaded to introduce the "unknown selector warning" if

• it's more clear that certain codes have changed in runes mode and so the warning is no longer silenced (i.e. reintroduce the context the other PR has)
• it prevents false positives from people having prose as part of the ignore comment (e.g. svelte-ignore a11y_x_y we handle this in the parent component). Unclear what a good heuristic for that is.

[Rich-Harris]

We absolutely need the unknown_code warning, it's a real flaw that it doesn't already exist. We also need an unused_svelte_ignore warning, though that can be a separate PR. (ESLint has both of these, and while TypeScript doesn't have a concept of 'warnings', it will yell at you for an unnecessary @ts-ignore comment — in an ideal world you'd be able to specify error codes.)

I hadn't considered the 'prose as part of the ignore comment' situation. I think we can solve that unambiguously with a small change — make the codes comma-separated rather than space-separated, and anything after the comma-separated list gets ignored:

<!-- svelte-ignore this_warning, that_warning because they're handled elsewhere -->

If we did that we'd want to tell people who did <!-- svelte-ignore this_warning that_warning --> to add a comma, which does theoretically add some ambiguity (that_warning could be the beginning of the prose section), but realistically it would be fine.

Adding the context to the no-longer-silenced warning feels like a mistake to me. For one thing, the unknown_code warning will be right next to it in 99% of cases — in other words the context is already present. And explicitly saying 'we know you tried to silence this warning but we're going to make you jump through some hoops anyway' feels aloof and annoying, whereas 'we didn't recognise this code, but here's our best guess as to what you meant' feels helpful.

[dummdidumm]

I think context is crucial for the migration story because it works in legacy mode and then you opt into runes for the component and suddenly there's a warning - and you're like "huh, what do you mean that code is unrecognized now?". Giving the hint that it was a backwards compat feat clues you into why this worked before and gives you a "ah thanks for thinking of that" feeling.

As for the heuristic: I would say commas are not necessary, instead treat every word that has a dash or underscore as a potential warning, and warn for everything that's not matching that's before the first non-code-looking word.
Example:
foo_bar baz-baz prose bla something_something will match foo_bar, baz-baz and something_something but only warn on foo_bar and baz-baz as unused. This should catch 99% of all cases because everyone is going to add the code(s) before the prose, but for those who don't we still catch the code but prevent false positives by not reporting words that happen to look like codes

[Rich-Harris]

Adjusted the warning message to make it clear that this is a 4 -> 5 change — there's now legacy_code alongside unknown_code.

The prose thing is still a TODO. I'd prefer the comma-separated list because it's ambiguous otherwise — is that_thing a typo, or the name of a local variable?:

<!-- svelte-ignore this_thing that_thing is not this_thing -->

ESLint uses comma-separated lists, so it's not like we're going rogue.

[dummdidumm]

Ok let's go with commas then

[dummdidumm]

(I'll do migration and a proposal for a simpler ignore traversal in a separate PR)

[Rich-Harris]

The lint task keeps hanging, which is very strange — I've rerun it a few times. I'm seeing this in the output:

Which seems like bollocks (and I can't reproduce it locally, oddly) but it makes me worried about merging this until we can guarantee it's not going to bork up future CI runs

[jrmajor]

As for „prose as part of the ignore comment” syntax — PHPStan just introduced error identifiers, and they use parentheses for that, which looks very natural and removes any ambiguity:

We might also want to explain why an error is ignored. The description has to be put in parentheses after the identifier:

// @phpstan-ignore offsetAccess.notFound (exists, set by a reference)
data_set($target[$segment], $segments, $value, $overwrite);

(from https://phpstan.org/blog/phpstan-1-11-errors-identifiers-phpstan-pro-reboot#error-identifiers)

[Rich-Harris]

Interesting. I think that's probably a reasonable constraint — further eliminates ambiguity, and encourages ecosystem-wide consistency. Thoughts @dummdidumm?

[dummdidumm]

I'm not sure if the braces bring us any additional value if we require all the error codes separated by commas upfront. We could add it alter if you want to add the reason right after the code