Skip to content

Avoid repetition on “use of unstable library feature 'rustc_private'” #45540

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.

Sign up for GitHub

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

Merged
bors merged 4 commits into from
Oct 28, 2017
Merged

Avoid repetition on “use of unstable library feature 'rustc_private'” #45540

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e16e30
fix #44953 - The “use of unstable library feature 'rustc_private'” er…
Oct 25, 2017
52d0e51
relax restriction - allow errors to be emmitted within macro spans, a…
Oct 26, 2017
304c8b1
implemented code review
Oct 28, 2017
bb0049b
fixed tidy error
Oct 28, 2017
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
29 changes: 26 additions & 3 deletions src/librustc/middle/stability.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use hir::def_id::{CrateNum, CRATE_DEF_INDEX, DefId, LOCAL_CRATE};
use ty::{self, TyCtxt};
use middle::privacy::AccessLevels;
use syntax::symbol::Symbol;
use syntax_pos::{Span, DUMMY_SP};
use syntax_pos::{Span, MultiSpan, DUMMY_SP};
use syntax::ast;
use syntax::ast::{NodeId, Attribute};
use syntax::feature_gate::{GateIssue, emit_feature_err, find_lang_feature_accepted_version};
Expand Down Expand Up @@ -597,8 +597,31 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
feature.as_str(), &r),
None => format!("use of unstable library feature '{}'", &feature)
};
emit_feature_err(&self.sess.parse_sess, &feature.as_str(), span,
GateIssue::Library(Some(issue)), &msg);


let msp: MultiSpan = span.into();
let cm = &self.sess.parse_sess.codemap();
let span_key =
Copy link
Contributor

@estebank estebank Oct 27, 2017

Choose a reason for hiding this comment

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

You can probably fit the following line on this one.

msp.primary_span().and_then(|sp:Span|
if sp != DUMMY_SP {
Copy link
Contributor

@estebank estebank Oct 27, 2017

Choose a reason for hiding this comment

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

Clean up the formatting for this entire block to avoid as having as much left padding.

let fname = cm.lookup_char_pos(sp.lo()).file.as_ref().name.clone();
if fname.starts_with("<") && fname.ends_with(" macros>") {
Copy link
Contributor

@estebank estebank Oct 27, 2017

Choose a reason for hiding this comment

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

This piece of code could be better as a method of cm.lookup_char_pos(sp.lo()).file.

Copy link
Contributor Author

@virgil-palanciuc virgil-palanciuc Oct 28, 2017

Choose a reason for hiding this comment

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

Not sure what you meant. Hopefully the way it's written now is better (well, it is better, I don't exactly know why I felt I needed to make it so complicated... )

None
} else {
Some(span)
}
} else {
None
}
);

let tuple = (None, span_key, msg.clone());
Copy link
Contributor

@estebank estebank Oct 27, 2017

Choose a reason for hiding this comment

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

Try to give tuple a more descriptive name. You can also get around that by just inserting the tuple inline ;)

let fresh = self.sess.one_time_diagnostics.borrow_mut().insert(tuple);
if fresh {
emit_feature_err(&self.sess.parse_sess, &feature.as_str(), span,
GateIssue::Library(Some(issue)), &msg);
}

}
Some(_) => {
// Stable APIs are always ok to call and deprecated APIs are
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/session/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub struct Session {
/// Set of (LintId, Option<Span>, message) tuples tracking lint
/// (sub)diagnostics that have been set once, but should not be set again,
/// in order to avoid redundantly verbose output (Issue #24690).
pub one_time_diagnostics: RefCell<FxHashSet<(lint::LintId, Option<Span>, String)>>,
pub one_time_diagnostics: RefCell<FxHashSet<(Option<lint::LintId>, Option<Span>, String)>>,
Copy link
Contributor

@estebank estebank Oct 27, 2017

Choose a reason for hiding this comment

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

Normally, instead of making the first tuple field an option, I would suggest making the FxHashSet accept a new struct with three fields diagnostic_id, span and message, with diagnostic_id being a new enum that takes either a LintId or a GateIssue. This way we can keep expanding one_time_diagnostics without the risk of having multiple different diagnostics with the same span message and no lint_id, but being different errors, from being incorrectly stopped, but because of the message being there, what you're doing is correct and unlikely to be made incorrect with a subsequent change.

pub plugin_llvm_passes: RefCell<Vec<String>>,
pub plugin_attributes: RefCell<Vec<(String, AttributeType)>>,
pub crate_types: RefCell<Vec<config::CrateType>>,
Expand Down Expand Up @@ -361,7 +361,7 @@ impl Session {
},
_ => {
let lint_id = lint::LintId::of(lint);
let id_span_message = (lint_id, span, message.to_owned());
let id_span_message = (Some(lint_id), span, message.to_owned());
let fresh = self.one_time_diagnostics.borrow_mut().insert(id_span_message);
if fresh {
do_method()
Expand Down