#2806 bugfix: Added checks for JS keywords in signatures. #2855
Conversation
crates/macro-support/src/parser.rs
Outdated
| "finally", | ||
| "function", | ||
| "import", | ||
| "instaceof", |
crates/macro-support/src/parser.rs
Outdated
|
|
||
| thread_local!(static ATTRS: AttributeParseState = Default::default()); | ||
|
|
||
| /// Javascript keywords which are not keywords in Rust. | ||
| const JS_KEYWORDS: [&str; 19] = [ |
|
Not sure if it's just a visual bug on my side or if it actually submitted the same review twice. No idea how that happened. Sorry for the spam if it did. |
|
@y21 Thanks for the review. Submitted fix. |
|
Personally I think this would be best solved in the JS code generation rather than by forbidding this in Rust. Would it be possible to move the fix there instead? |
|
Sure, I will change that. |
petrstudynka
force-pushed
the
js-keywords-prefix-fix
branch
from
e6f08f9
to
0f754ed
Compare
|
I found out those keywords do not conflict when there are in defined as method name. So thats the reason for the if-else in the last commit. Otherwise those idents are mangled with underscore. |
|
Can some tests be added for this to ensure that it works? |
|
Ok looks great, thanks! |
| _ => "", | ||
| }; | ||
| let name = if prefix.is_empty() && opts.method().is_none() && is_js_keyword(js_name) { | ||
| format!("_{}", js_name) |
There was a problem hiding this comment.
This should not silently replace the name of exported and imported identifiers where they make a semantic difference. Accessing them by bracket instead of dot syntax sounds like a reasonable workaround if the user actually does use a js keyword. I.e. wasm["class"] instead of silently changing to wasm._class against explicit annotation in
There was a problem hiding this comment.
That sounds like a reasonable solution that should be easy to implement, would you like to make a PR? Happy to review it.
Added checks in macro crate for those cases downstream crate uses idents, which would result in invalid JS code generation.