fix(namer): escape, rather than strip, non-ASCII ident. characters

[ErichDonGubler]

Escape non-ASCII identifier characters with write!(…, "u{:04x}", …), surrounding with _ as appropriate. This solves (1) a debugging issue where stripped characters would otherwise be invisible, and (2) failure to re-validate that stripped identifiers didn't start with an ASCII digit.

I've confirmed that this fixes bug 1978197 on the Firefox side.

Testing

Added a regression test.

Squash or Rebase?

squashplz

Checklist

• If this contains user-facing changes, add a CHANGELOG.md entry.

[andyleiserson]

+1 for not generating illegal identifiers 😄

[andyleiserson]

Not important, since you've covered it with snapshot tests, but one thing I've noticed about naga/wgpu is that we don't have a lot of unit tests, and this behavior seems like a good candidate for unit testing. (But as I said, not important, I don't think it's worth going back and changing/adding the tests, this is more a reminder to be thinking about unit tests in general.)

[ErichDonGubler]

Agreed on the scope; I also think it would be nice to use several snapshot-ish unit tests in follow-up work, just to make it easier to reason about some changes.

[ErichDonGubler]

Just filed https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=144433 to see if this breaks anything. AFAIK it's unusual for us to change stuff like this, though I'm not concerned about anything concrete.

[jimblandy]

I'm not really comfortable with the way this affects so many identifiers that don't contain non-ASCII characters. Naga does not promise to preserve identifier names at all; we could just name everything e1, e2. There's no benefit to us guaranteeing that the original identifier can be reconstructed from the identifiers we generate.

[jimblandy]

The only job of Namer::sanitize is to produce a valid identifier prefix quickly and simply. Preserving the original name is just for our convenience in debugging; there is no contract. We should not double the size of this code just to delete characters safely.

For example, a better fix might be to delete almost all of that function, and then say, if string works as-is, return it; otherwise, return "e". That's the kind of direction we want to be headed here, not growing our own identifier mangling syntax.

[cwfitzgerald]

Preserving the original name is just for our convenience in debugging; there is no contract.

I will note that we really should try to be as close as possible with names - if we stray too far, we will take another hit to debuggability, and we alreayd aren't great with that.

[ErichDonGubler]

Opened a community chat about this: https://matrix.to/#/!FZyQrssSlHEZqrYcOb:matrix.org/$O9OsIA7qYHd0M0FpFnnPkdqPF3MuJOkQ9Oz8i7lY2sE?via=matrix.org&via=mozilla.org&via=tchncs.de

[ErichDonGubler]

From Wednesday's triage notes:

JB: No progress, but will close today

too long bro

[ErichDonGubler]

I'm not sure how the mipmap-query test would be affected by this. @cwfitzgerald, @andyleiserson, @teoxoy, any ideas? 💦

[cwfitzgerald]

It has been intermittently failing for a hot minute, I haven't investigated why.