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

#[rustc_pass_by_value] cleanup #93363

Merged
merged 2 commits into from
Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/rustc_ast_pretty/src/pprust/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ pub trait PrintState<'a>: std::ops::Deref<Target = pp::Printer> + std::ops::Dere
&mut self,
macro_def: &ast::MacroDef,
ident: &Ident,
sp: &Span,
sp: Span,
print_visibility: impl FnOnce(&mut Self),
) {
let (kw, has_bang) = if macro_def.macro_rules {
Expand All @@ -623,7 +623,7 @@ pub trait PrintState<'a>: std::ops::Deref<Target = pp::Printer> + std::ops::Dere
macro_def.body.delim(),
&macro_def.body.inner_tokens(),
true,
*sp,
sp,
);
if macro_def.body.need_semicolon() {
self.word(";");
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast_pretty/src/pprust/state/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ impl<'a> State<'a> {
}
}
ast::ItemKind::MacroDef(ref macro_def) => {
self.print_mac_def(macro_def, &item.ident, &item.span, |state| {
self.print_mac_def(macro_def, &item.ident, item.span, |state| {
state.print_visibility(&item.vis)
});
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
),
rustc_attr!(
rustc_pass_by_value, Normal,
template!(Word), WarnFollowing,
template!(Word), ErrorFollowing,
"#[rustc_pass_by_value] is used to mark types that must be passed by value instead of reference."
),
BuiltinAttribute {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_pretty/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ impl<'a> State<'a> {
self.ann.nested(self, Nested::Body(body));
}
hir::ItemKind::Macro(ref macro_def) => {
self.print_mac_def(macro_def, &item.ident, &item.span, |state| {
self.print_mac_def(macro_def, &item.ident, item.span, |state| {
state.print_visibility(&item.vis)
});
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/non_fmt_panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,5 +336,5 @@ fn is_arg_inside_call(arg: Span, call: Span) -> bool {
// panic call in the source file, to avoid invalid suggestions when macros are involved.
// We specifically check for the spans to not be identical, as that happens sometimes when
// proc_macros lie about spans and apply the same span to all the tokens they produce.
call.contains(arg) && !call.source_equal(&arg)
call.contains(arg) && !call.source_equal(arg)
}
4 changes: 2 additions & 2 deletions compiler/rustc_lint/src/pass_by_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ fn gen_args(cx: &LateContext<'_>, segment: &PathSegment<'_>) -> String {
.map(|arg| match arg {
GenericArg::Lifetime(lt) => lt.name.ident().to_string(),
GenericArg::Type(ty) => {
cx.tcx.sess.source_map().span_to_snippet(ty.span).unwrap_or_default()
cx.tcx.sess.source_map().span_to_snippet(ty.span).unwrap_or_else(|_| "_".into())
}
GenericArg::Const(c) => {
cx.tcx.sess.source_map().span_to_snippet(c.span).unwrap_or_default()
cx.tcx.sess.source_map().span_to_snippet(c.span).unwrap_or_else(|_| "_".into())
}
GenericArg::Infer(_) => String::from("_"),
})
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_middle/src/mir/spanview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ where
}

/// Format a string showing the start line and column, and end line and column within a file.
pub fn source_range_no_file<'tcx>(tcx: TyCtxt<'tcx>, span: &Span) -> String {
pub fn source_range_no_file<'tcx>(tcx: TyCtxt<'tcx>, span: Span) -> String {
let source_map = tcx.sess.source_map();
let start = source_map.lookup_char_pos(span.lo());
let end = source_map.lookup_char_pos(span.hi());
Expand Down Expand Up @@ -629,7 +629,7 @@ fn tooltip<'tcx>(
let mut text = Vec::new();
text.push(format!("{}: {}:", spanview_id, &source_map.span_to_embeddable_string(span)));
for statement in statements {
let source_range = source_range_no_file(tcx, &statement.source_info.span);
let source_range = source_range_no_file(tcx, statement.source_info.span);
text.push(format!(
"\n{}{}: {}: {:?}",
TOOLTIP_INDENT,
Expand All @@ -639,7 +639,7 @@ fn tooltip<'tcx>(
));
}
if let Some(term) = terminator {
let source_range = source_range_no_file(tcx, &term.source_info.span);
let source_range = source_range_no_file(tcx, term.source_info.span);
text.push(format!(
"\n{}{}: {}: {:?}",
TOOLTIP_INDENT,
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/coverage/spans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ impl CoverageStatement {
let stmt = &mir_body[bb].statements[stmt_index];
format!(
"{}: @{}[{}]: {:?}",
source_range_no_file(tcx, &span),
source_range_no_file(tcx, span),
bb.index(),
stmt_index,
stmt
Expand All @@ -38,7 +38,7 @@ impl CoverageStatement {
let term = mir_body[bb].terminator();
format!(
"{}: @{}.{}: {:?}",
source_range_no_file(tcx, &span),
source_range_no_file(tcx, span),
bb.index(),
term_type(&term.kind),
term.kind
Expand Down Expand Up @@ -155,7 +155,7 @@ impl CoverageSpan {
pub fn format<'tcx>(&self, tcx: TyCtxt<'tcx>, mir_body: &mir::Body<'tcx>) -> String {
format!(
"{}\n {}",
source_range_no_file(tcx, &self.span),
source_range_no_file(tcx, self.span),
self.format_coverage_statements(tcx, mir_body).replace('\n', "\n "),
)
}
Expand Down
24 changes: 12 additions & 12 deletions compiler/rustc_span/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ impl Span {

#[inline]
/// Returns `true` if `hi == lo`.
pub fn is_empty(&self) -> bool {
pub fn is_empty(self) -> bool {
let span = self.data_untracked();
span.hi == span.lo
}
Expand Down Expand Up @@ -639,7 +639,7 @@ impl Span {
///
/// Use this instead of `==` when either span could be generated code,
/// and you only care that they point to the same bytes of source text.
pub fn source_equal(&self, other: &Span) -> bool {
pub fn source_equal(self, other: Span) -> bool {
let span = self.data();
let other = other.data();
span.lo == other.lo && span.hi == other.hi
Expand Down Expand Up @@ -680,17 +680,17 @@ impl Span {
}

#[inline]
pub fn rust_2015(&self) -> bool {
pub fn rust_2015(self) -> bool {
self.edition() == edition::Edition::Edition2015
}

#[inline]
pub fn rust_2018(&self) -> bool {
pub fn rust_2018(self) -> bool {
self.edition() >= edition::Edition::Edition2018
}

#[inline]
pub fn rust_2021(&self) -> bool {
pub fn rust_2021(self) -> bool {
self.edition() >= edition::Edition::Edition2021
}

Expand All @@ -711,15 +711,15 @@ impl Span {
/// Checks if a span is "internal" to a macro in which `#[unstable]`
/// items can be used (that is, a macro marked with
/// `#[allow_internal_unstable]`).
pub fn allows_unstable(&self, feature: Symbol) -> bool {
pub fn allows_unstable(self, feature: Symbol) -> bool {
self.ctxt()
.outer_expn_data()
.allow_internal_unstable
.map_or(false, |features| features.iter().any(|&f| f == feature))
}

/// Checks if this span arises from a compiler desugaring of kind `kind`.
pub fn is_desugaring(&self, kind: DesugaringKind) -> bool {
pub fn is_desugaring(self, kind: DesugaringKind) -> bool {
match self.ctxt().outer_expn_data().kind {
ExpnKind::Desugaring(k) => k == kind,
_ => false,
Expand All @@ -728,7 +728,7 @@ impl Span {

/// Returns the compiler desugaring that created this span, or `None`
/// if this span is not from a desugaring.
pub fn desugaring_kind(&self) -> Option<DesugaringKind> {
pub fn desugaring_kind(self) -> Option<DesugaringKind> {
match self.ctxt().outer_expn_data().kind {
ExpnKind::Desugaring(k) => Some(k),
_ => None,
Expand All @@ -738,7 +738,7 @@ impl Span {
/// Checks if a span is "internal" to a macro in which `unsafe`
/// can be used without triggering the `unsafe_code` lint.
// (that is, a macro marked with `#[allow_internal_unsafe]`).
pub fn allows_unsafe(&self) -> bool {
pub fn allows_unsafe(self) -> bool {
self.ctxt().outer_expn_data().allow_internal_unsafe
}

Expand All @@ -751,7 +751,7 @@ impl Span {
return None;
}

let is_recursive = expn_data.call_site.source_equal(&prev_span);
let is_recursive = expn_data.call_site.source_equal(prev_span);

prev_span = self;
self = expn_data.call_site;
Expand Down Expand Up @@ -865,13 +865,13 @@ impl Span {

/// Equivalent of `Span::call_site` from the proc macro API,
/// except that the location is taken from the `self` span.
pub fn with_call_site_ctxt(&self, expn_id: ExpnId) -> Span {
pub fn with_call_site_ctxt(self, expn_id: ExpnId) -> Span {
self.with_ctxt_from_mark(expn_id, Transparency::Transparent)
}

/// Equivalent of `Span::mixed_site` from the proc macro API,
/// except that the location is taken from the `self` span.
pub fn with_mixed_site_ctxt(&self, expn_id: ExpnId) -> Span {
pub fn with_mixed_site_ctxt(self, expn_id: ExpnId) -> Span {
self.with_ctxt_from_mark(expn_id, Transparency::SemiTransparent)
}

Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_span/src/span_encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,15 @@ use rustc_data_structures::fx::FxIndexSet;
/// using the callback `SPAN_TRACK` to access the query engine.
///
#[derive(Clone, Copy, Eq, PartialEq, Hash)]
// FIXME(@lcnr): Enable this attribute once the bootstrap
// compiler knows of `rustc_pass_by_value`.
//
// Right now, this lint would only trigger when compiling the
// stage 2 compiler, which is fairly annoying as there are
// a lot of places using `&Span` right now. After the next bootstrap bump,
// the lint will already trigger when using stage 1, which is a lot less annoying.
//
// #[cfg_attr(not(bootstrap), rustc_pass_by_value)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// #[cfg_attr(not(bootstrap), rustc_pass_by_value)]
#[cfg_attr(not(bootstrap), rustc_pass_by_value)]

Why doesn't this work? Isn't not(bootstrap) supposed to specifically address this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

&Span is used incredibly frequently, so fixing it takes a while. As long as rustc_pass_by_value is ignored in the bootstrap compiler, it isn't as nice to deal with that.

It means that the lint will only trigger when compiling the stage 2 compiler, which most people don't tend to do before submitting their PR, causing CI to break. So it would work, but would imo be too annoying to be worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you say in the comment that not all uses of Span are fixed, and it's tedious to fix them until it can be done at stage 0-1 when stage 0-1 supports #[rustc_pass_by_value]?
Right now it's entirely unclear from the comment why the attribute is commented out.

pub struct Span {
base_or_index: u32,
len_or_tag: u16,
Expand Down