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.

Sign up for GitHub

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

Jump to bottom

Unify TokResult and ResultAnyMacro #36669

Merged
merged 4 commits into from
Sep 27, 2016
Merged

Unify TokResult and ResultAnyMacro #36669

merged 4 commits into from
Sep 27, 2016

Conversation

jseyfried
Copy link
Contributor

Fixes #36641.
r? @nrc

jseyfried
jseyfried commented Sep 23, 2016
View reviewed changes
src/libsyntax/ext/base.rs
fn make_items(mut self: Box<Self>) -> Option<SmallVector<P<ast::Item>>> {
if self.parser.sess.span_diagnostic.has_errors() {
return Some(SmallVector::zero());
}
Copy link
Contributor Author

@jseyfried jseyfried Sep 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nrc is there a reason we treat items specially here by bailing out early if there have been errors?

This PR currently doesn't bail out early if there have been errors, but if there's a reason for it I could bail out on all expansions or just on items like now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably do this for all cases - this would happen if we can't parse what the proc macro has produced - in that case, there doesn't seem any point in including the expanded code only to get the same errors again (it is probably only in items since that is the only case where the parser errored out later, or possibly just because that is the only case I tested :-( )

Copy link
Contributor Author

@jseyfried jseyfried Sep 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought at this point we haven't parsed what the proc macro has produced yet.

Wouldn't we be bailing out here if there have been any errors for any reason -- even totally related to the macro in question?

jseyfried
jseyfried commented Sep 24, 2016
View reviewed changes
src/libsyntax/parse/parser.rs
if allow_semi && self.token == token::Semi {
// We allow semicolons at the end of expressions -- e.g. the semicolon in
// `macro_rules! m { () => { panic!(); } }` isn't parsed by `.parse_expr()`,
// but `m!()` is allowed in expression positions (c.f. issue #34706).
Copy link
Contributor Author

@jseyfried jseyfried Sep 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the motivation for this -- might be a good idea not to allow in macros 2.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you get unexpected behaviour without this - something to do with expressions in statement context and having the same behaviour for macros as other expressions. Oh and allowing the same macro to be used in both expression and statement position (and possibly the weirdness about whether a macro is a block or not).

Copy link
Contributor Author

@jseyfried jseyfried Sep 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a macro author wants their macro to be usable in an expression context, couldn't they just not include the trailing semicolon? Afaik, the trailing semicolon isn't needed for the macro to be usable in a statement context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, ignoring the trailing semicolon creates unexpected (to me) behavior today -- for example, in this gist I would expect the expansion to have the unit type instead of i32.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(compare to this gist, which compiles)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I amended this PR so that ProcMacro and AttrProcMacro do not allow trailing semicolons in expression positions.

nrc
nrc reviewed Sep 25, 2016
View reviewed changes
src/libsyntax/parse/parser.rs
@@ -6172,6 +6174,45 @@ impl<'a> Parser<'a> {
}
}

pub fn parse_expansion(&mut self, kind: ExpansionKind) -> PResult<'a, Expansion> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this might be better in ext somewhere rather than in the Parser. IIRC the parser is fairly ignorant of macro stuff at the moment but is kind of huge and hard to understand, I think it might serve us well to minimise adding macro stuff. What do you think?

Copy link
Contributor Author

@jseyfried jseyfried Sep 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should minimize adding stuff to the parser in general, but I think this is a simple and small enough function with similar enough structure to the other parse_* functions to warrant being in parser.rs.

It's not that important though -- I'd be OK moving it to expand.rs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of wary of this because it introduces new types to the parser module and it doesn't seem to need to be a method - it doesn't use any of Parser's private API afaict, so seems like it would be fine in expand.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense -- I amended this PR to move parse_expansion to expand.rs.

@bors
Copy link
Contributor

bors commented Sep 25, 2016

☔ The latest upstream changes (presumably #36616) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc
Copy link
Member

nrc commented Sep 26, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 26, 2016

📌 Commit df0e4bf has been approved by nrc

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 26, 2016
Rollup merge of rust-lang#36669 - jseyfried:refactor_tok_result, r=nrc
ae77889 
Unify `TokResult` and `ResultAnyMacro`

Fixes rust-lang#36641.
r? @nrc
@sophiajt sophiajt mentioned this pull request Sep 26, 2016
Closed
sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 27, 2016
Rollup merge of rust-lang#36669 - jseyfried:refactor_tok_result, r=nrc
5cc9fb9 
Unify `TokResult` and `ResultAnyMacro`

Fixes rust-lang#36641.
r? @nrc
@sophiajt sophiajt mentioned this pull request Sep 27, 2016
Merged
bors added a commit that referenced this pull request Sep 27, 2016
@bors
Auto merge of #36764 - jonathandturner:rollup, r=jonathandturner
ec7679b 
Rollup of 14 pull requests

- Successful merges: #36563, #36574, #36586, #36662, #36663, #36669, #36676, #36721, #36723, #36727, #36729, #36742, #36754, #36756
- Failed merges:
@bors bors merged commit df0e4bf into rust-lang:master Sep 27, 2016
@jseyfried jseyfried deleted the refactor_tok_result branch October 16, 2016 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrc nrc nrc left review comments

Assignees

@nrc nrc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jseyfried @bors @nrc