Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle the possibility that the class name is in its own Group #2159

Merged
merged 4 commits into from
May 26, 2020

Conversation

rrbutani
Copy link
Contributor

Closes #2158.

I'm not sure if this is the best way to go about fixing this or if there are other places where this kind of thing should be added.

If it's helpful, @dtolnay's fix for proc-macro-hack is here.

@rrbutani
Copy link
Contributor Author

rrbutani commented May 26, 2020

I think the remaining #[wasm_bindgen] invocation that fails under the latest nightly is breaking because of syn.

syn seems to assume that entire expressions will be a part of a single Group (trailer_expr in expr.rs) which leads to the bit that parses the body of functions (Block::parse_within in stmt.rs) checking if the fragment of the expression that was returned from parse_stmt (this calls stmt_expr which calls expr_earlyunary_exprtrailer_expr) needs a semicolon (using requires_terminator in expr.rs). requires_terminator isn't Group aware but it doesn't matter; either way the function would correctly report that expressions do need to be followed by a semicolon when they're the last thing in a block. And so, we get an error.

I think the right thing to do here is to update the expr parse machinery to not stop on Groups (without just throwing away the Groups). Just ditching the check for Groups in trailer_expr seems to do the trick for me, but I'm not sure if this is the best way to do things/actually fixes the issue properly (I'm not at all familiar with syn's internals).

@dtolnay does the above sound at all correct / should I open an issue on syn's repo?

@alexcrichton
Copy link
Contributor

Thanks for this! Poking around a bit I think it might be best to be a bit more general with these fixes rather than being so case-specific in these few locations. I'm a bit lost though in the final error and how we could work around and/or fix it...

@alexcrichton
Copy link
Contributor

If this parse error seems to originate from syn though then it seems reasonable to file an issue upstream about that.

@rrbutani rrbutani marked this pull request as ready for review May 26, 2020 19:30
@rrbutani
Copy link
Contributor Author

@alexcrichton I made the suggested changes.

That last error doesn't seem to happen anymore in CI, but I have no idea why. The fix that was pushed to syn doesn't seem like it fixes our case (we don't have a :: in the function name).

It actually also still fails with the same error for me locally.

@rrbutani
Copy link
Contributor Author

Oh whoops; just noticed you pinned the nightly version.

I'll open an issue on syn.

@alexcrichton
Copy link
Contributor

In any case this seems like a good change to land regardless, so I'm going to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

js-sys fails to build with nightlies on and after 2020-05-25
2 participants