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

Avoid repeated interning of env!("CFG_RELEASE") #117188

Merged
merged 10 commits into from
Oct 27, 2023
4 changes: 0 additions & 4 deletions compiler/rustc_attr/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@ pub const VERSION_PLACEHOLDER: &str = "CURRENT_RUSTC_VERSION";

pub const CURRENT_RUSTC_VERSION: &str = env!("CFG_RELEASE");

pub fn rust_version_symbol() -> Symbol {
Symbol::intern(CURRENT_RUSTC_VERSION)
}

pub fn is_builtin_attr(attr: &Attribute) -> bool {
attr.is_doc_comment() || attr.ident().is_some_and(|ident| is_builtin_attr_name(ident.name))
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![feature(never_type)]
#![feature(proc_macro_diagnostic)]
#![feature(proc_macro_span)]
#![feature(proc_macro_tracked_env)]
#![allow(rustc::default_hash_types)]
#![deny(rustc::untranslatable_diagnostic)]
#![deny(rustc::diagnostic_outside_of_impl)]
Expand Down
158 changes: 126 additions & 32 deletions compiler/rustc_macros/src/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use proc_macro2::{Span, TokenStream};
use quote::quote;
use std::collections::HashMap;
use syn::parse::{Parse, ParseStream, Result};
use syn::{braced, punctuated::Punctuated, Ident, LitStr, Token};
use syn::{braced, punctuated::Punctuated, Expr, Ident, Lit, LitStr, Macro, Token};

#[cfg(test)]
mod tests;
Expand All @@ -53,21 +53,46 @@ impl Parse for Keyword {

struct Symbol {
name: Ident,
value: Option<LitStr>,
value: Value,
}

enum Value {
SameAsName,
String(LitStr),
Env(LitStr, Macro),
Unsupported(Expr),
}

impl Parse for Symbol {
fn parse(input: ParseStream<'_>) -> Result<Self> {
let name = input.parse()?;
let value = match input.parse::<Token![:]>() {
Ok(_) => Some(input.parse()?),
Err(_) => None,
};
let colon_token: Option<Token![:]> = input.parse()?;
let value = if colon_token.is_some() { input.parse()? } else { Value::SameAsName };

Ok(Symbol { name, value })
}
}

impl Parse for Value {
fn parse(input: ParseStream<'_>) -> Result<Self> {
let expr: Expr = input.parse()?;
match &expr {
Expr::Lit(expr) => {
if let Lit::Str(lit) = &expr.lit {
return Ok(Value::String(lit.clone()));
}
}
Expr::Macro(expr) => {
if expr.mac.path.is_ident("env") && let Ok(lit) = expr.mac.parse_body() {
return Ok(Value::Env(lit, expr.mac.clone()));
}
}
_ => {}
}
Ok(Value::Unsupported(expr))
}
}

struct Input {
keywords: Punctuated<Keyword, Token![,]>,
symbols: Punctuated<Symbol, Token![,]>,
Expand Down Expand Up @@ -111,6 +136,37 @@ pub fn symbols(input: TokenStream) -> TokenStream {
output
}

struct Preinterned {
idx: u32,
span_of_name: Span,
}

struct Entries {
map: HashMap<String, Preinterned>,
}

impl Entries {
fn with_capacity(capacity: usize) -> Self {
Entries { map: HashMap::with_capacity(capacity) }
}

fn insert(&mut self, span: Span, str: &str, errors: &mut Errors) -> u32 {
if let Some(prev) = self.map.get(str) {
errors.error(span, format!("Symbol `{str}` is duplicated"));
errors.error(prev.span_of_name, "location of previous definition".to_string());
prev.idx
} else {
let idx = self.len();
self.map.insert(str.to_string(), Preinterned { idx, span_of_name: span });
idx
}
}

fn len(&self) -> u32 {
u32::try_from(self.map.len()).expect("way too many symbols")
}
}

fn symbols_with_errors(input: TokenStream) -> (TokenStream, Vec<syn::Error>) {
let mut errors = Errors::default();

Expand All @@ -127,20 +183,9 @@ fn symbols_with_errors(input: TokenStream) -> (TokenStream, Vec<syn::Error>) {
let mut keyword_stream = quote! {};
let mut symbols_stream = quote! {};
let mut prefill_stream = quote! {};
let mut counter = 0u32;
let mut keys =
HashMap::<String, Span>::with_capacity(input.keywords.len() + input.symbols.len() + 10);
let mut entries = Entries::with_capacity(input.keywords.len() + input.symbols.len() + 10);
let mut prev_key: Option<(Span, String)> = None;

let mut check_dup = |span: Span, str: &str, errors: &mut Errors| {
if let Some(prev_span) = keys.get(str) {
errors.error(span, format!("Symbol `{str}` is duplicated"));
errors.error(*prev_span, "location of previous definition".to_string());
} else {
keys.insert(str.to_string(), span);
}
};

let mut check_order = |span: Span, str: &str, errors: &mut Errors| {
if let Some((prev_span, ref prev_str)) = prev_key {
if str < prev_str {
Expand All @@ -156,49 +201,98 @@ fn symbols_with_errors(input: TokenStream) -> (TokenStream, Vec<syn::Error>) {
let name = &keyword.name;
let value = &keyword.value;
let value_string = value.value();
check_dup(keyword.name.span(), &value_string, &mut errors);
let idx = entries.insert(keyword.name.span(), &value_string, &mut errors);
prefill_stream.extend(quote! {
#value,
});
keyword_stream.extend(quote! {
pub const #name: Symbol = Symbol::new(#counter);
pub const #name: Symbol = Symbol::new(#idx);
});
counter += 1;
}

// Generate the listed symbols.
for symbol in input.symbols.iter() {
let name = &symbol.name;
check_order(symbol.name.span(), &name.to_string(), &mut errors);

let value = match &symbol.value {
Some(value) => value.value(),
None => name.to_string(),
Value::SameAsName => name.to_string(),
Value::String(lit) => lit.value(),
Value::Env(..) => continue, // in another loop below
Value::Unsupported(expr) => {
errors.list.push(syn::Error::new_spanned(
expr,
concat!(
"unsupported expression for symbol value; implement support for this in ",
file!(),
),
));
continue;
}
};
check_dup(symbol.name.span(), &value, &mut errors);
check_order(symbol.name.span(), &name.to_string(), &mut errors);
let idx = entries.insert(symbol.name.span(), &value, &mut errors);

prefill_stream.extend(quote! {
#value,
});
symbols_stream.extend(quote! {
pub const #name: Symbol = Symbol::new(#counter);
pub const #name: Symbol = Symbol::new(#idx);
});
counter += 1;
}

// Generate symbols for the strings "0", "1", ..., "9".
let digits_base = counter;
counter += 10;
for n in 0..10 {
let n = n.to_string();
check_dup(Span::call_site(), &n, &mut errors);
entries.insert(Span::call_site(), &n, &mut errors);
prefill_stream.extend(quote! {
#n,
});
}

// Symbols whose value comes from an environment variable. It's allowed for
// these to have the same value as another symbol.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this allowed? In practice it won't be a problem because no other pre-interned symbol will match the version number, but in principle if one did it could certainly lead to incorrect comparisons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you share an example of how it might go wrong?

This implementation is intended to handle that situation correctly. I can't think of how having 2 Rust names for the same symbol would be bad.

Note that they will have the same SymbolIndex.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I misread the comment. I thought it was possible for two identical strings to end up with different symbol values. But the prev.idx case below covers the case where an env var has the same value as another symbol. Two different names for the same string/symbol should indeed be fine. Sorry for the noise.

for symbol in &input.symbols {
let (env_var, expr) = match &symbol.value {
Value::Env(lit, expr) => (lit, expr),
Value::SameAsName | Value::String(_) | Value::Unsupported(_) => continue,
};

if !proc_macro::is_available() {
errors.error(
Span::call_site(),
"proc_macro::tracked_env is not available in unit test".to_owned(),
);
break;
}

let value = match proc_macro::tracked_env::var(env_var.value()) {
Ok(value) => value,
Err(err) => {
errors.list.push(syn::Error::new_spanned(expr, err));
continue;
}
};

let idx = if let Some(prev) = entries.map.get(&value) {
prev.idx
} else {
prefill_stream.extend(quote! {
#value,
});
entries.insert(symbol.name.span(), &value, &mut errors)
};

let name = &symbol.name;
symbols_stream.extend(quote! {
pub const #name: Symbol = Symbol::new(#idx);
});
}

let symbol_digits_base = entries.map["0"].idx;
let preinterned_symbols_count = entries.len();
let output = quote! {
const SYMBOL_DIGITS_BASE: u32 = #digits_base;
const PREINTERNED_SYMBOLS_COUNT: u32 = #counter;
const SYMBOL_DIGITS_BASE: u32 = #symbol_digits_base;
const PREINTERNED_SYMBOLS_COUNT: u32 = #preinterned_symbols_count;

#[doc(hidden)]
#[allow(non_upper_case_globals)]
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_macros/src/symbols/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ fn test_symbols() {

let body_tokens = m.mac.tokens.clone();

test_symbols_macro(body_tokens, &[]);
test_symbols_macro(body_tokens, &["proc_macro::tracked_env is not available in unit test"]);
}

fn test_symbols_macro(input: TokenStream, expected_errors: &[&str]) {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_passes/src/lib_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
//! collect them instead.

use rustc_ast::Attribute;
use rustc_attr::{rust_version_symbol, VERSION_PLACEHOLDER};
use rustc_attr::VERSION_PLACEHOLDER;
use rustc_hir::intravisit::Visitor;
use rustc_middle::hir::nested_filter;
use rustc_middle::middle::lib_features::LibFeatures;
Expand Down Expand Up @@ -59,7 +59,7 @@ impl<'tcx> LibFeatureCollector<'tcx> {
if let Some(s) = since
&& s.as_str() == VERSION_PLACEHOLDER
{
since = Some(rust_version_symbol());
since = Some(sym::env_CFG_RELEASE);
}

if let Some(feature) = feature {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_passes/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

use crate::errors;
use rustc_attr::{
self as attr, rust_version_symbol, ConstStability, Since, Stability, StabilityLevel, Unstable,
UnstableReason, VERSION_PLACEHOLDER,
self as attr, ConstStability, Since, Stability, StabilityLevel, Unstable, UnstableReason,
VERSION_PLACEHOLDER,
};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap};
use rustc_hir as hir;
Expand Down Expand Up @@ -1115,7 +1115,7 @@ fn unnecessary_stable_feature_lint(
mut since: Symbol,
) {
if since.as_str() == VERSION_PLACEHOLDER {
since = rust_version_symbol();
since = sym::env_CFG_RELEASE;
}
tcx.emit_spanned_lint(
lint::builtin::STABLE_FEATURES,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,7 @@ symbols! {
encode,
end,
env,
env_CFG_RELEASE: env!("CFG_RELEASE"),
eprint_macro,
eprintln_macro,
eq,
Expand Down
Loading