Skip to content

Conversation

@lnicola
Copy link
Member

@lnicola lnicola commented Jan 6, 2023

No description provided.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2023
let expanded = match get_env_inner(db, arg_id, &key) {
None => quote! { #DOLLAR_CRATE::option::Option::None::<&str> },
Some(s) => quote! { #DOLLAR_CRATE::option::Some(#s) },
Some(s) => quote! { #DOLLAR_CRATE::option::Option::Some(#s) },
Copy link
Member Author

@lnicola lnicola Jan 6, 2023

Choose a reason for hiding this comment

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

@Veykril are you sure ::core::option::Option wouldn't work better here? I think we fail to expand a couple of these in analysis-stats because we don't support $crate properly (or do we? and is $crate really correct?).

CC #13329

Copy link
Member

Choose a reason for hiding this comment

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

and is $crate really correct?

The option_env! macro is defined in core, so I would expect it to be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we're not handling it correctly, then.

Copy link
Member

Choose a reason for hiding this comment

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

Odd, we should be handling $crate for these properly 🤔

Copy link
Member Author

@lnicola lnicola Jan 6, 2023

Choose a reason for hiding this comment

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

Yeah, we don't resolve it (see the three instances in version.rs), while it works with ::core::option::Option. I wonder if we don't already have an issue for something like this.

Copy link
Member

@Veykril Veykril Jan 6, 2023

Choose a reason for hiding this comment

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

Maybe, I mean I am looking into rewriting the token map stuff (though I don't have a lot of time to put into r-a currently), and I imagine this will also require me to change how we handle $crate as this involves rewriting/adding hygiene proper handling. So that might fix it in the end as well.

@Veykril
Copy link
Member

Veykril commented Jan 6, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2023

📌 Commit 9f6567f has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 6, 2023

⌛ Testing commit 9f6567f with merge f1a9901...

@lnicola lnicola changed the title fix: Fix option_env expansion minor: Fix option_env expansion Jan 6, 2023
@bors
Copy link
Contributor

bors commented Jan 6, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f1a9901 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Jan 6, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing f1a9901 to master...

@bors
Copy link
Contributor

bors commented Jan 6, 2023

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

@bors bors merged commit f1a9901 into rust-lang:master Jan 6, 2023
@lnicola lnicola deleted the option-env branch January 6, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants