-
Notifications
You must be signed in to change notification settings - Fork 13.3k
give some suggestion for returning anonymous enum #100988
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -880,6 +880,97 @@ impl<'a> Parser<'a> { | |
} | ||
} | ||
|
||
pub(super) fn check_anonymous_enum(&mut self, span: Span, tys: &[P<Ty>]) -> PResult<'a, ()> { | ||
use std::fmt::Write; | ||
if tys.len() <= 1 { | ||
return Ok(()); | ||
} | ||
|
||
fn variant_name_and_ty<'a>( | ||
ty: &'a Ty, | ||
lifetimes: &mut FxHashSet<&'a str>, | ||
) -> Option<(String, String)> { | ||
match &ty.kind { | ||
TyKind::Path(_, path) => { | ||
let mut name = String::new(); | ||
let mut ty_string = String::new(); | ||
|
||
if let Some(seg) = path.segments.iter().last() { | ||
name.push_str(seg.ident.name.as_str()); | ||
ty_string.push_str(seg.ident.name.as_str()); | ||
|
||
if let Some(_args) = &seg.args { | ||
ty_string.push('<'); | ||
ty_string.push_str("..."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we print the whole type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's more complicated, for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you propose, we can use |
||
ty_string.push('>'); | ||
} | ||
} | ||
|
||
Some((name, ty_string)) | ||
} | ||
TyKind::Rptr(lifetime, ty) => { | ||
if let Some((mut name, ty)) = variant_name_and_ty(&ty.ty, lifetimes) { | ||
name.push_str("Ref"); | ||
let lifetime = | ||
lifetime.as_ref().map(|l| l.ident.name.as_str()).unwrap_or("'lifetime"); | ||
lifetimes.insert(lifetime); | ||
Some((name, format!("&{} {}", lifetime, ty))) | ||
} else { | ||
None | ||
} | ||
} | ||
_ => None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wouldn't work for something like If you don't want to support every |
||
} | ||
} | ||
|
||
let mut err = self.struct_span_err(span, "anonymous enums are not supported"); | ||
let mut variant_content = String::new(); | ||
let mut variant_name_set = FxHashSet::default(); | ||
let mut lifetimes = FxHashSet::default(); | ||
tys.iter().for_each(|ty| { | ||
let name_ty = variant_name_and_ty(ty, &mut lifetimes); | ||
if let Some((variant_name, ty)) = name_ty { | ||
let variant_name = crate::utils::to_camel_case(&variant_name); | ||
if !variant_name_set.contains(&variant_name) { | ||
let _ = writeln!( | ||
&mut variant_content, | ||
" {variant_name}({ty})", | ||
variant_name = variant_name, | ||
ty = ty | ||
); | ||
variant_name_set.insert(variant_name); | ||
} | ||
} | ||
}); | ||
|
||
let mut suggestion_code = String::new(); | ||
suggestion_code.push_str("enum SomeEnum"); | ||
if lifetimes.len() > 0 { | ||
suggestion_code.push_str("<"); | ||
#[allow(rustc::potential_query_instability)] | ||
let mut iter = lifetimes.into_iter(); | ||
if let Some(lifetime) = iter.next() { | ||
suggestion_code.push_str(lifetime); | ||
while let Some(lifetime) = iter.next() { | ||
suggestion_code.push_str(","); | ||
suggestion_code.push_str(lifetime); | ||
} | ||
} | ||
suggestion_code.push_str(">"); | ||
} | ||
suggestion_code.push_str("{\n"); | ||
suggestion_code.push_str(&variant_content); | ||
suggestion_code.push_str("}\n"); | ||
err.span_suggestion( | ||
span, | ||
"consider using enum as return type", | ||
"SomeEnum", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to account for the lifetimes that were mentioned. In the test included, this would end up being |
||
Applicability::HasPlaceholders, | ||
) | ||
.note(suggestion_code); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of the note with the enum, we could instead use a
|
||
Err(err) | ||
} | ||
|
||
/// Eats and discards tokens until one of `kets` is encountered. Respects token trees, | ||
/// passes through any errors encountered. Used for error recovery. | ||
pub(super) fn eat_to_tokens(&mut self, kets: &[&TokenKind]) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ pub(super) enum AllowPlus { | |
No, | ||
} | ||
|
||
#[derive(PartialEq)] | ||
#[derive(PartialEq, Clone, Copy)] | ||
pub(super) enum RecoverQPath { | ||
Yes, | ||
No, | ||
|
@@ -199,14 +199,32 @@ impl<'a> Parser<'a> { | |
) -> PResult<'a, FnRetTy> { | ||
Ok(if self.eat(&token::RArrow) { | ||
// FIXME(Centril): Can we unconditionally `allow_plus`? | ||
let ty = self.parse_ty_common( | ||
allow_plus, | ||
AllowCVariadic::No, | ||
recover_qpath, | ||
recover_return_sign, | ||
None, | ||
RecoverQuestionMark::Yes, | ||
)?; | ||
|
||
let mut tys = vec![]; | ||
let lo = self.token.span; | ||
loop { | ||
Comment on lines
+203
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue with this that we shouldn't recover this only for return types, we should be doing this further down, in |
||
let ty = self.parse_ty_common( | ||
allow_plus, | ||
AllowCVariadic::No, | ||
recover_qpath, | ||
recover_return_sign, | ||
None, | ||
RecoverQuestionMark::Yes, | ||
)?; | ||
tys.push(ty); | ||
// maybe a `|` for fn type of closure, `|a: &dyn Fn(i32) -> bool| { ... }` | ||
if self.check_noexpect(&token::BinOp(token::Or)) | ||
&& self.look_ahead(1, |tok| tok == &token::OpenDelim(token::Delimiter::Brace)) | ||
{ | ||
break; | ||
} | ||
if !self.eat_noexpect(&token::BinOp(token::Or)) { | ||
break; | ||
} | ||
} | ||
let span = lo.to(self.prev_token.span); | ||
self.check_anonymous_enum(span, &tys)?; | ||
let ty = tys.into_iter().next().unwrap(); | ||
FnRetTy::Ty(ty) | ||
} else if recover_return_sign.can_recover(&self.token.kind) { | ||
// Don't `eat` to prevent `=>` from being added as an expected token which isn't | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||
/// copied from rustc_lint | ||||
|
||||
/// Some unicode characters *have* case, are considered upper case or lower case, but they *can't* | ||||
/// be upper cased or lower cased. For the purposes of the lint suggestion, we care about being able | ||||
/// to change the char's case. | ||||
fn char_has_case(c: char) -> bool { | ||||
let mut l = c.to_lowercase(); | ||||
let mut u = c.to_uppercase(); | ||||
while let Some(l) = l.next() { | ||||
match u.next() { | ||||
Some(u) if l != u => return true, | ||||
_ => {} | ||||
} | ||||
} | ||||
u.next().is_some() | ||||
} | ||||
|
||||
pub fn to_camel_case(s: &str) -> String { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but it's in crate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could move this to a shared dependency of the two, like |
||||
s.trim_matches('_') | ||||
.split('_') | ||||
.filter(|component| !component.is_empty()) | ||||
.map(|component| { | ||||
let mut camel_cased_component = String::new(); | ||||
|
||||
let mut new_word = true; | ||||
let mut prev_is_lower_case = true; | ||||
|
||||
for c in component.chars() { | ||||
// Preserve the case if an uppercase letter follows a lowercase letter, so that | ||||
// `camelCase` is converted to `CamelCase`. | ||||
if prev_is_lower_case && c.is_uppercase() { | ||||
new_word = true; | ||||
} | ||||
|
||||
if new_word { | ||||
camel_cased_component.extend(c.to_uppercase()); | ||||
} else { | ||||
camel_cased_component.extend(c.to_lowercase()); | ||||
} | ||||
|
||||
prev_is_lower_case = c.is_lowercase(); | ||||
new_word = false; | ||||
} | ||||
|
||||
camel_cased_component | ||||
}) | ||||
.fold((String::new(), None), |(acc, prev): (String, Option<String>), next| { | ||||
// separate two components with an underscore if their boundary cannot | ||||
// be distinguished using an uppercase/lowercase case distinction | ||||
let join = if let Some(prev) = prev { | ||||
let l = prev.chars().last().unwrap(); | ||||
let f = next.chars().next().unwrap(); | ||||
!char_has_case(l) && !char_has_case(f) | ||||
} else { | ||||
false | ||||
}; | ||||
(acc + if join { "_" } else { "" } + &next, Some(next)) | ||||
}) | ||||
.0 | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
struct Foo {} | ||
|
||
fn foo<'a>() -> i32 | Vec<i32> | &str | &'a String | Foo { | ||
//~^ ERROR: anonymous enums are not supported | ||
} | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
error: anonymous enums are not supported | ||
--> $DIR/issue-100741-return-anonymous-enum.rs:3:17 | ||
| | ||
LL | fn foo<'a>() -> i32 | Vec<i32> | &str | &'a String | Foo { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using enum as return type: `SomeEnum` | ||
| | ||
= note: enum SomeEnum<'lifetime,'a>{ | ||
I32(i32) | ||
Vec(Vec<...>) | ||
StrRef(&'lifetime str) | ||
StringRef(&'a String) | ||
Foo(Foo) | ||
} | ||
|
||
|
||
error: aborting due to previous error | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like the only thing we need here is the
name
and forty_string
format!("{ty}")
is enough, after folding the anon lifetimes to be named as'lifetime
, in way similar to what is done here:rust/compiler/rustc_middle/src/ty/erase_regions.rs
Lines 56 to 69 in c3fce8e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emmm, the
TyKind
in rustc_ast doesn't implementDisplay
, if it's nesscery to implementDisplay
, I will submit a new pull request to do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly certain there's a way to
pprint
TyKind
, but it isn't throughDisplay
directly 🤔