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

env! syntax extension changes #8362

Closed
wants to merge 1 commit into from
Closed

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Aug 7, 2013

env! aborts compilation of the specified environment variable is not
defined and takes an optional second argument containing a custom
error message. option_env! creates an Option<&'static str> containing
the value of the environment variable.

There are no run-pass tests that check the behavior when the environment
variable is defined since the test framework doesn't support setting
environment variables at compile time as opposed to runtime. However,
both env! and option_env! are used inside of rustc itself, which should
act as a sufficient test.

Fixes #2248.

pub fn version(argv0: &str) {
let vers = match env!("CFG_VERSION") {
Some(vers) => vers,
None => "unknown_version"
Copy link
Member

Choose a reason for hiding this comment

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

went from "unknown version" to "unknown_version"

@alexcrichton
Copy link
Member

Just tiny nitpicks, otherwise r+, thanks!

@alexcrichton
Copy link
Member

Could the commits be squashed together for just one?

@sfackler
Copy link
Member Author

sfackler commented Aug 7, 2013

Incidentally, it seems to me from a usability perspective that syntax extensions are processed too early in the pipeline. Dealing with the raw AST causes some super weird stuff, like std::option::Some(s) being an expr_call_global and std::option::None being an expr_path. It would be nice if processing happened after everything was parsed and resolved so you could explicitly declare a variant_constructor or something like that.

I realize that it's probably not something that would be reasonable to do at this point. A version of syntax::ext::build that worked at a higher level of abstraction could probably get you most of the way there. At the very least, an extra output mode of --pretty that's a tabified version of the ast::Crate would be helpful to see what the AST of something you want to make looks like. I spent a long time thinking that std::option::Some(s) was supposed to be an expr_struct until I added a printfln!(crate) to rustc::driver::pretty_print_input and manually tabified the output of a trivial program. I think this kind of support will be especially important once syntax extensions are dynamically loadable and it becomes more reasonable for people not already extremely familiar with the compiler's internals to write them.

@sfackler
Copy link
Member Author

sfackler commented Aug 7, 2013

@alexcrichton squashed

@graydon
Copy link
Contributor

graydon commented Aug 7, 2013

Incidentally these sorts of ast details / differences are exactly what the quasiquoter is there to gloss over. Though I realize it's incomplete and not always effective, I'd suggest directing energies at improving it.

@sfackler
Copy link
Member Author

sfackler commented Aug 7, 2013

I didn't realize that the quasiquoter even existed! I'll play around with that, thanks!

@huonw
Copy link
Member

huonw commented Aug 7, 2013

Re the quasiquoter: #7727 is open as the bug for improving it.

Also, I feel like in many cases one wants a compile time error if the env-var doesn't exist. Maybe env!() should error if it's missing, and option_env!() should return an Option<&'static str>. (Or some other names.)

@sfackler
Copy link
Member Author

sfackler commented Aug 7, 2013

That seems pretty reasonable to me. env!() should probably take an optional second parameter to customize the error message. There's also the common use case of overriding something with an environment variable, but I don't think that the cost of doing option_env!("FOO").unwrap_or_default("default") is high enough to justify a third version.

@alexcrichton
Copy link
Member

In case anyone's curios, @sfackler and I discussed this on IRC and agree with @huonw that there should be two versions:

  • env! which takes two arguments, the second is an optional message to print if the variable isn't set. This results in a compile-time failure if the environment variable isn't defined.
  • env_option! takes only one argument and returns an option of a string.

@sfackler
Copy link
Member Author

sfackler commented Aug 8, 2013

@alexcrichton r?

@@ -0,0 +1,13 @@
// file at the top-level directory of this distribution and at
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 this lost the first line of the license

@alexcrichton
Copy link
Member

Would you mind changing the error-pattern tests to using //~ ERROR: instead? That makes sure that you emit the right span on the error message and that there's no other error messages at the same time.

Also, I'm a little worried about requiring these variables for rustc. Personally more than once I've compiled rustc by hand (mostly to check syntax/types). I never use the resulting library, but it would be a little annoying to have to make sure these variables are defined. That being said, I'm probably only one of few who does this, and I could have some shell startup stuff which sets these variables, so I'm not that worried. Basically r+ with the test changes.

And if you're at it, would you mind changing the copyright years on the files to 2013 instead of 2012? Thanks again for doing this!

env! aborts compilation of the specified environment variable is not
defined and takes an optional second argument containing a custom
error message. option_env! creates an Option<&'static str> containing
the value of the environment variable.

There are no run-pass tests that check the behavior when the environment
variable is defined since the test framework doesn't support setting
environment variables at compile time as opposed to runtime. However,
both env! and option_env! are used inside of rustc itself, which should
act as a sufficient test.

Close rust-lang#2248
bors added a commit that referenced this pull request Aug 9, 2013
env! aborts compilation of the specified environment variable is not
defined and takes an optional second argument containing a custom
error message. option_env! creates an Option<&'static str> containing
the value of the environment variable.

There are no run-pass tests that check the behavior when the environment
variable is defined since the test framework doesn't support setting
environment variables at compile time as opposed to runtime. However,
both env! and option_env! are used inside of rustc itself, which should
act as a sufficient test.

Fixes #2248.
@bors bors closed this Aug 9, 2013
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.

Make syntax::expand::expand_syntax_ext return an option
5 participants