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

rustc: Stricter matching of --cfg options #31530

Merged
merged 2 commits into from
Feb 15, 2016

Conversation

dirk
Copy link
Contributor

@dirk dirk commented Feb 10, 2016

Fixes #31497.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@dirk
Copy link
Contributor Author

dirk commented Feb 10, 2016

A couple questions:

  1. Is this the right way to approach this? Is comparing the strings after parsing too late, or should we make the parser function itself errors if there's any input string left after the parsed Ident?
  2. How should tests of this behavior be done?

@dirk dirk changed the title Stricter matching of --cfg options on rustc rustc: Stricter matching of --cfg options Feb 10, 2016
@alexcrichton
Copy link
Member

I think that this still wants to parse a meta instead of an ident to we can still pass, for example, --cfg 'feature = "foo"'.

For tests you can add some compile-fail tests using the // compile-flags feature to help pass arguments down to rustc (invalid ones), and then you can use // error-pattern to match the output

@dirk
Copy link
Contributor Author

dirk commented Feb 11, 2016

@alexcrichton: If that's the case, then would it make sense to do some basic validation (perhaps with a simple visitor pattern) of the tree produced by the meta parser?

@alexcrichton
Copy link
Member

It may make the most sense to just make sure there are no more tokens in the parser once a meta item is parsed (although I'm not sure how to do this). I think what's happening here is we're successfully parsing a meta item and we're just trashing the remaining tokens (when they should be considered invalid)

@dirk
Copy link
Contributor Author

dirk commented Feb 11, 2016

Oooh, good point! I'll look into that.

Includes compile-fail test to check that it fails on incomplete
`--cfg` matches.

Fixes rust-lang#31497.
@dirk dirk force-pushed the dirk/restrict-matching-of-cfg-opts branch from dd56425 to b122064 Compare February 15, 2016 00:45
@dirk
Copy link
Contributor Author

dirk commented Feb 15, 2016

@alexcrichton: Comments addressed and compile-fail test added!

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 15, 2016
@alexcrichton
Copy link
Member

@bors: r+ b122064

Thanks!

@dirk
Copy link
Contributor Author

dirk commented Feb 15, 2016

@alexcrichton: Thank you for pointing me in the right direction! 😄 That got me to take another at the parser it uses under the hood and discover the very handy .is_eof() function.

@bors
Copy link
Contributor

bors commented Feb 15, 2016

⌛ Testing commit b122064 with merge b6ca777...

@dirk
Copy link
Contributor Author

dirk commented Feb 15, 2016

Looks like there's something in the build script that is causing error-index-generator to be interpreted as being passed to --cfg, which causes the compiler to error, see here.

@bors
Copy link
Contributor

bors commented Feb 15, 2016

💔 Test failed - auto-linux-musl-64-opt

@alexcrichton
Copy link
Member

Ah that's because that's legitimately an invalid cfg! That should probably change to error_index_generator

@dirk
Copy link
Contributor Author

dirk commented Feb 15, 2016

@alexcrichton: Huh, so I can see a --cfg error-index-generator on the end of the call to rustc to build src/error-index-generator/main.rs, but can't figure out where in the Makefile it's coming from.

Edit: Nevermind, found it here.

This is because the tool compiler passes the name of the tool
as a command line `--cfg`. The improved session config parser
is stricter and no longer permits invalid meta items (such as
"error-index-generator").
@dirk
Copy link
Contributor Author

dirk commented Feb 15, 2016

@alexcrichton: Turns out the fix was a simple (and probably overdue) rename! See 2766e25.

@dirk
Copy link
Contributor Author

dirk commented Feb 15, 2016

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Feb 15, 2016
@alexcrichton
Copy link
Member

@bors: r+ 2766e25

@bors
Copy link
Contributor

bors commented Feb 15, 2016

⌛ Testing commit 2766e25 with merge 17d284b...

bors added a commit that referenced this pull request Feb 15, 2016
@bors bors merged commit 2766e25 into rust-lang:master Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants