fix(namer): escape, rather than strip, non-ASCII ident. characters #7995
Conversation
ErichDonGubler
added
type: bug
ErichDonGubler
force-pushed
the
escape-utf-idents
branch
2 times, most recently
from
e6b5270 to
0108909
Compare
ErichDonGubler
force-pushed
the
escape-utf-idents
branch
from
0108909 to
09a15fd
Compare
| if !s.is_empty() && !had_underscore_at_end { | ||
| s.push('_'); | ||
| } | ||
| write!(s, "u{:04x}_", c as u32).unwrap(); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
| @@ -5,11 +5,11 @@ precision highp int; | |||
|
|
|||
| layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; | |||
|
|
|||
| struct _atomic_compare_exchange_resultSint4_ { | |||
| struct _atomic_compare_exchange_result_u003c_Sint_u002c_4_u003e { | |||
There was a problem hiding this comment.
It's slightly unfortunate to use so many characters for an internally-generated type name. Maybe there could be a rule like "any number of consecutive :<>, characters are mapped to a single underscore"?
There was a problem hiding this comment.
Maybe something like 3a4c931?
There was a problem hiding this comment.
Yes, that looks great to me.
|
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. |
There was a problem hiding this comment.
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.
|
The only job of For example, a better fix might be to delete almost all of that function, and then say, if |
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](https://bugzilla.mozilla.org/show_bug.cgi?id=1978197) on the
Firefox side.
ErichDonGubler
force-pushed
the
escape-utf-idents
branch
from
09a15fd to
63dcc20
Compare
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
CHANGELOG.mdentry.