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

Add a compile_error! macro to libstd #1695

Merged
merged 2 commits into from
Mar 28, 2017

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Aug 1, 2016

@sfackler
Copy link
Member

sfackler commented Aug 1, 2016

I'd love something like this. I'm currently forcing type errors to do these kinds of checks, but that's not a great user experience: https://github.com/sfackler/rust-postgres/blob/306a6a9d187167b864ed5b28147b5da176549f58/src/feature_check.rs

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 1, 2016
@alexcrichton
Copy link
Member

Tagging as T-libs, but also relevant to the @rust-lang/lang team as it would essentially be adding a new macro the language (e.g. libcore wouldn't define it)

@Stebalien
Copy link
Contributor

This would make macro libraries like horrorshow suck significantly less. However, it would actually be nice to pass a custom span to compile_error!. This way DSLs (like horrorshow) could tell the user where their mistake is.

@golddranks
Copy link

What about error message localizations (as there has been some discussion recently about supporting localized compiler error messages)? Even if compile_error! wouldn't support localization in the end, I'd like them to be mentioned so it won't go unconsidered.

@sgrif
Copy link
Contributor Author

sgrif commented Aug 1, 2016

@golddranks I'd imagine that localization will include some sort of way to detect them using cfg attributes, so it could be accomplished via:

#[cfg(locale = "en_US")]
compile_error!("LOOKOUT!");

#[cfg(locale = "de")]
compile_error!("ACHTUNG!");

The span given for the failure should be the invocation of the `compile_error!`
macro. The macro must take exactly one argument, which is a string literal. The
macro will then call `span_err` with the provided message on the expansion
context, and will not expand to any further code.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth accepting a format string rather than a plain string literal? Errors seem like the kind of thing that would be commonly customised?

Is it also worth accepting alternative spans some how? That seems like a useful tool, but I don't have an idea for how it would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that this entirely at compile time, I'm not sure that there's anything that we could format that wouldn't be equally handled by concat!.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way I can think of to accept alternative spans would be to take a file/line number/char number triple, which I think is fine as an option if folks are in favor of it.

@sfackler sfackler self-assigned this Aug 1, 2016
@kennytm
Copy link
Member

kennytm commented Aug 2, 2016

cc #1146.

On nightly there are the static_assert and compile_msg crates.

@nikomatsakis
Copy link
Contributor

I am in favor of this general idea (along with some kind of static-assert, but I guess that (could be) a separate thing). The question of how to "specify" a span is interesting -- I am presuming this error would be reported during macro expansion?

@sciyoshi
Copy link

sciyoshi commented Aug 4, 2016

👍, I think this is a good first step towards improving the readability of macro errors in general. How would custom spans be specified? @sgrif any thoughts on also including a compile_warning! macro?

@sgrif
Copy link
Contributor Author

sgrif commented Aug 7, 2016

I am presuming this error would be reported during macro expansion?

Correct. The implementation would essentially just be a procedural macro which passes its single argument to cx.span_err.

The question of how to "specify" a span is interesting

I'm really not sure that there's a compelling use case for that, as you really don't generally have span information without procedural macros in the first place.

any thoughts on also including a compile_warning! macro?

Seems like a reasonable addition if others are in favor of it.

@Stebalien
Copy link
Contributor

I'm really not sure that there's a compelling use case for that, as you really don't generally have span information without procedural macros in the first place.

Is the span information not attached to the tokens passed to a macro? I'd like to be able point out which token in the macro input caused the macro expansion to fail (and why).

@briansmith
Copy link

Semantically, this would be just a special case of static_assert!(false, msg), right? Does it make sense to have both this and static_assert!? IMO, no, because they overlap too much. Consider, in particular, static_assert!(cfg!(locale = "de"), "ACHTUNG!"), in constrast to the example mentioned above.

@eddyb
Copy link
Member

eddyb commented Aug 20, 2016

@briansmith static_assert!, to actually evaluate its first argument, would have to run after type-checking instead of erroring at expansion time, which could result in other derived errors showing up first.
It might not be a bad idea to only have a "static assert", although consider:

// A potential desugaring, assuming CTFE that turns panics into compile errors.
const _: () = assert!(cfg!(locale = "de"), "ACHTUNG!");

I'm not sure if being able to implement static_assert! like that makes the macro unnecessary, but it certainly would affect the design.

@shepmaster
Copy link
Member

I always imagined that something like this would be implemented as a panic! inside of a const fn (which is what it appears @eddyb mentions).

@eddyb
Copy link
Member

eddyb commented Sep 15, 2016

@shepmaster I would like @solson to confirm it, but I'm pretty sure miri already supports panic! as a compiler error (but not unwinding or running destructors).

@solson
Copy link
Member

solson commented Sep 15, 2016

@shepmaster @eddyb Miri can't reach the internal panic machinery inside panic!() quite yet, but turning uncaught unwinds into compiler errors was always the plan.

@DanielKeep
Copy link

Just popping by to note that the "force a type error on a constant" trick doesn't work in macros any more: the compiler just points at the macro invocation, and never shows the erroneous construct.

Right now, I have no way of catching and reporting input mistakes to the user. Can we get something, anything for this?

@aturon
Copy link
Member

aturon commented Feb 1, 2017

Ping @sfackler

@sfackler
Copy link
Member

sfackler commented Feb 1, 2017

Ah yes hello! I'd like to FCP to merge - while this will be implementable in the future via third party syntax extensions, it's a common enough workflow that I feel it's good to natively support it.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 1, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@kennytm
Copy link
Member

kennytm commented Feb 2, 2017

To someone having permission:
@rfcbot concern compile_warning

Do we include compile_warning! as well? #1695 (comment) #1695 (comment)

@sfackler
Copy link
Member

sfackler commented Feb 6, 2017

Ah good point. I'd probably say we add it since it's a pretty straightforward addition.

@iliekturtles
Copy link

What about no_std libraries? Should/can the macros be in libcore?

@solson
Copy link
Member

solson commented Feb 19, 2017

@iliekturtles Good call, it seems to me like they can and should be in libcore.

@sgrif
Copy link
Contributor Author

sgrif commented Feb 26, 2017

I think compile_warning! should be left to a separate RFC if it is deemed useful. It has more questions around it than compile_error! does (how does it interact with #[deny(warnings)], is it #[allow]able? How is the lint specified), and has a less obvious use case. I've updated the RFC to specify libcore instead of libstd

@KalitaAlexey
Copy link

@sgrif,
As you want to add the macro to the libcore, you should rename the PR to mention libcore instead of libstd.

@alexcrichton
Copy link
Member

ping @BurntSushi (checkbox)

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 14, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Mar 14, 2017
@CasualX
Copy link

CasualX commented Mar 23, 2017

As a neat workaround you can (ab)use the env! macro :) your warning message is unlikely to exist as an env var so you will get an error.

I asked if this feature existed some time ago in /r/rust

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 24, 2017

The final comment period is now complete.

@sfackler
Copy link
Member

The comment period has elapsed, and this RFC is accepted! Tracking issue: rust-lang/rust#40872

@Centril Centril added A-macros-libstd Proposals that introduce new standard library macros A-diagnostics Proposals relating to diagnostics (error messages). labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Proposals relating to diagnostics (error messages). A-macros-libstd Proposals that introduce new standard library macros final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.