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

give some suggestion for returning anonymous enum #100988

Closed
wants to merge 1 commit into from
Closed
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
1 change: 1 addition & 0 deletions compiler/rustc_parse/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub const MACRO_ARGUMENTS: Option<&str> = Some("macro arguments");
pub mod parser;
use parser::{emit_unclosed_delims, make_unclosed_delims_error, Parser};
pub mod lexer;
mod utils;
pub mod validate_attr;

// A bunch of utility functions of the form `parse_<thing>_from_<source>`
Expand Down
91 changes: 91 additions & 0 deletions compiler/rustc_parse/src/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

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 for ty_string format!("{ty}") is enough, after folding the anon lifetimes to be named as 'lifetime, in way similar to what is done here:

fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
// because late-bound regions affect subtyping, we can't
// erase the bound/free distinction, but we can replace
// all free regions with 'erased.
//
// Note that we *CAN* replace early-bound regions -- the
// type system never "sees" those, they get substituted
// away. In codegen, they will always be erased to 'erased
// whenever a substitution occurs.
match *r {
ty::ReLateBound(..) => r,
_ => self.tcx.lifetimes.re_erased,
}
}

Copy link
Contributor Author

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 implement Display, if it's nesscery to implement Display, I will submit a new pull request to do that.

Copy link
Contributor

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 through Display directly 🤔

ty_string.push_str(seg.ident.name.as_str());

if let Some(_args) = &seg.args {
ty_string.push('<');
ty_string.push_str("...");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we print the whole type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's more complicated, for example Vec<impl Send> | Vec<(i32, i64)>. We can not make a enum variant name for these type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you propose, we can use Variant{N} as the variant name

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

This wouldn't work for something like -> [i32; 10] | Vec<i32>, right?

If you don't want to support every TyKind, you can at least provide a Variant{N} name so that the suggested enum makes syntactic sense.

}
}

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",
Copy link
Contributor

Choose a reason for hiding this comment

The 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 '_, 'a.

Applicability::HasPlaceholders,
)
.note(suggestion_code);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the note with the enum, we could instead use a multipart_suggestion and pass in the span for the space right at the start of the item, so that the output can be

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 {
   |                     ^          ^      ^            ^ not supported in types
   |
help: consider using enum as return type
   |
LL + enum SomeEnum<'lifetime,'a>{
LL +     I32(i32)
LL +     Vec(Vec<i32>)
LL +     StrRef(&'lifetime str)
LL +     StringRef(&'a String)
LL +     Foo(Foo)
LL + }
LL ~ fn foo<'a>() -> SomeEnum<'_, '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]) {
Expand Down
36 changes: 27 additions & 9 deletions compiler/rustc_parse/src/parser/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub(super) enum AllowPlus {
No,
}

#[derive(PartialEq)]
#[derive(PartialEq, Clone, Copy)]
pub(super) enum RecoverQPath {
Yes,
No,
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 parse_ty_common, maybe adding another argument to it to explicitly state what should be coming after it (to support things like let foo: A | B = ...;).

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
Expand Down
60 changes: 60 additions & 0 deletions compiler/rustc_parse/src/utils.rs
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

fn to_camel_case(s: &str) -> String {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it's in crate rustc_lint. I think it's weird to make rustc_parse depend rustc_lint. Should we create a crate rustc_utils for these functions like to_camel_case

Copy link
Contributor

Choose a reason for hiding this comment

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

We could move this to a shared dependency of the two, like rustc_errors.

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
}
7 changes: 7 additions & 0 deletions src/test/ui/return/issue-100741-return-anonymous-enum.rs
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() {}
17 changes: 17 additions & 0 deletions src/test/ui/return/issue-100741-return-anonymous-enum.stderr
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