Error on invalid import attr position #4522
Conversation
|
Judging by other PRs CI status, quite a few of these errors are coming simply from new Rust version and would happen on main branch too. I guess that needs to be fixed upstream first. |
There was a problem hiding this comment.
Changelog entry missing, otherwise LGTM!
I've had the unpleasant experience in the past that a lot of people out there rely on the fact that attributes don't error for very strange reasons. Sometimes these reasons were valid enough that we had to revert turning on errors for certain attributes.
It would be good if you could go through blame and find out why errors were disabled in the first place. In any case, if we don't find a good reason (I predict we won't) I'm happy to break things like these and say sorry later.
I have a suspicion it's rather that wasm-bindgen used to not validate attributes at all in the beginning, so any validations became opt-in on case-by-case basis to avoid breaking existing code. Worst case scenario, we can break it in the new major version whenever we make one, but yeah, if there's no good reason to keep them from validating today, it's worth exploring. |
|
Rebase and changelog entry on this? |
The reference tests for imports used invalid syntax, and because we don't error on attributes in invalid position by default, it was silently passing anyway.
I've changed the import attributes to error by default (by the way, why do we not do this for all attributes? it seems like a pretty common footgun), fixed the test, changed it to
--target webso that we could actually see imported files in the JS snapshot, and updated snapshots.