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

Accept strings as values of attributes #12467

Closed
wants to merge 2 commits into from

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Feb 22, 2014

This patch is the first step to solve #11886. It allows quoted strings as values of attributes and uses this syntax for lang items, while keeping the old syntax lang = "foo" valid. I've created another branch, which changes all lang = "foo" attributes to lang("foo"), because this only works after this PR got merged and is in the current snapshot (It seems like #[cfg(stage0)] does not work any more). The relevant commit can be found in my fork: fhahn@41527f7

With this patch, quoted strings are accepted as values of all attributes, not only for lang attributes, but I'm not sure if this is intended.

Also, I want to add a few tests, because it seems like there are not tests for lang at the moment.

@huonw
Copy link
Member

huonw commented Feb 22, 2014

It seems like #[cfg(stage0)] does not work any more

I'm pretty sure it does...


Am I reading this correctly that it basically converts #[foo("bar")] to be equivalent to #[foo(bar)]?

for attribute in attrs.iter() {
match attribute.name_str_pair() {
Some((ref key, ref value)) if key.equiv(&("lang")) => {
// Raise error after snapshot landed
//session.err(format!("`lang = {}` was replaced by `lang({})`",
Copy link
Member

Choose a reason for hiding this comment

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

This could be format!(" ... {0} ... {0}", *value) to avoid the repeat.

@fhahn
Copy link
Contributor Author

fhahn commented Feb 22, 2014

I'm not sure how to use #[cfg(not(stage0))] to skip parsing of the #[lang("str_eq")] attribute with the snapshot compiler. I tried the following code, which raises an error with the snapshot compiler, because an ident is expected instead of a quoted string.

/// Bytewise slice equality
#[cfg(not(stage0))]
#[cfg(not(test))]
#[lang("str_eq")]
#[inline]
pub fn eq_slice(a: &str, b: &str) -> bool {
    eq_slice_(a, b)
}

Am I reading this correctly that it basically converts #[foo("bar")] to be equivalent to #[foo(bar)]?

Yes, both forms are equivalent, both set the value of foo to the string bar. However I'm not sure what the intention behind using quoted strings instead of idents is. For the lang items idents would work as well.

@alexcrichton
Copy link
Member

This will need to add support for foo("bar") and then switch it in a later commit once we get a snapshot.

I'm a little surprised that foo("bar") is the same as foo(bar) because what about foo("something with spaces")? Our attribute language has a nice symmetry right now where everything inside of the parens is just a continuation of the same grammar for what's inside the brackets. We're starting to violate that symmetry by disallowing foo = "bar" at the root, and I think for now we're keeping foo = "bar" inside of parens, so it's already a little off.

Regardless, though, I think that foo("bar") should be an equivalent form of foo = "bar" in that it's a name/value pair, not an inner list of attributes.

@fhahn
Copy link
Contributor Author

fhahn commented Feb 22, 2014

From the discussion in #11886 it wasn't entirely clear to me in which way quoted strings in attributes should be accepted.

I'll update the code to generate name/value pairs for foo("bar") attributes. This should be the only place in attributes, that accepts quoted strings?

@huonw
Copy link
Member

huonw commented Feb 22, 2014

(General point: specifically for lang, I'd like #[lang(fail_)] etc. rather than #[lang("fail_")]; similar to #[inline(...)].)

@fhahn
Copy link
Contributor Author

fhahn commented Feb 23, 2014

It seems like there is no real consensus on this issue. Is it valid for lang items to be something different than an identifier? What was the reason for using quoted strings in the first place?

If the foo("bar") syntax is intended to replace all instances of foo="bar", than this patch could be a first step. Changing some attributes from foo="bar" to foo("bar") while keeping others could seem confusing.

@alexcrichton
Copy link
Member

Lang items are foo = "bar" because that was the original chosen syntax. That syntax does not accept an identifier as the value, is just so happens that all values are indeed identifiers.

I'm under the impression that we're changing all instances of #[foo = "bar"] to #[foo("bar")], not instances such as #[cfg(target_os = "win32")].

I am personally unsure of whether we want #[cfg(target_os = "win32")] to be equivalent to #[cfg(target_os("win32"))].

@huonw
Copy link
Member

huonw commented Feb 23, 2014

Is it valid for lang items to be something different than an identifier?

We get to choose the names of the lang items, so we can choose them to always be valid identifiers. :)

@brson
Copy link
Contributor

brson commented Feb 26, 2014

Don't want to derail this discussion, but the lang item syntax in particular was ill-chosen since lang items can easily be idents.

@brson
Copy link
Contributor

brson commented Feb 26, 2014

Also, if #12538 gets merged then the impetus for this change is less since we'll be keeping the brackets.

That doesn't mean we shouldn't do this change, but should probably step back and think about it again.

@alexcrichton
Copy link
Member

We ended up deciding to close #11886 for now, so I'm going to close this as well.

@fhahn fhahn deleted the issue-11886-change-lang-style branch March 13, 2014 22:33
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
fix: Fix match to if let assist for wildcard pats
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.

4 participants