give some suggestion for returning anonymous enum by LYF1999 · Pull Request #100988 · rust-lang/rust

LYF1999 · 2022-08-25T07:59:36Z

give some suggestion for returning anonymous enum

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)
           }

fix #100741
r? @estebank

LYF1999 · 2022-08-25T08:00:31Z

should we create a crate for some util functions like to_camel_case?

estebank · 2022-09-01T17:11:40Z

compiler/rustc_parse/src/parser/diagnostics.rs

+            "SomeEnum",
+            Applicability::HasPlaceholders,
+        )
+        .note(suggestion_code);


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> { |

estebank · 2022-09-01T17:12:39Z

compiler/rustc_parse/src/parser/diagnostics.rs

+        err.span_suggestion(
+            span,
+            "consider using enum as return type",
+            "SomeEnum",


You need to account for the lifetimes that were mentioned. In the test included, this would end up being '_, 'a.

estebank · 2022-09-01T17:13:17Z

compiler/rustc_parse/src/parser/diagnostics.rs

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


Why don't we print the whole type?

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

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

estebank · 2022-09-01T17:20:42Z

compiler/rustc_parse/src/parser/diagnostics.rs

+                    let mut ty_string = String::new();
+
+                    if let Some(seg) = path.segments.iter().last() {
+                        name.push_str(seg.ident.name.as_str());


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:

rust/compiler/rustc_middle/src/ty/erase_regions.rs

Lines 56 to 69 in c3fce8e

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,

}

}

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.

I'm fairly certain there's a way to pprint TyKind, but it isn't through Display directly 🤔

estebank · 2022-09-01T17:22:40Z

compiler/rustc_parse/src/parser/diagnostics.rs

+                        None
+                    }
+                }
+                _ => None,


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.

estebank · 2022-09-01T17:38:23Z

compiler/rustc_parse/src/parser/ty.rs

+            let mut tys = vec![];
+            let lo = self.token.span;
+            loop {


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 = ...;).

estebank · 2022-09-01T17:39:57Z

compiler/rustc_parse/src/utils.rs

+    u.next().is_some()
+}
+
+pub fn to_camel_case(s: &str) -> String {


rust/compiler/rustc_lint/src/nonstandard_style.rs

Line 90 in dcd5177

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

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

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

bors · 2023-01-26T09:34:07Z

☔ The latest upstream changes (presumably #107314) made this pull request unmergeable. Please resolve the merge conflicts.

Noratrieb · 2023-03-14T11:29:53Z

any updates here? we have to be very careful to not recover on the happy path as to not cause further regressions

LYF1999 · 2023-03-14T13:39:02Z

sorry, I am not working on it currently

bors · 2023-05-02T13:17:34Z

☔ The latest upstream changes (presumably #109128) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2023-05-15T09:37:08Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

rust-highfive assigned estebank Aug 25, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2022

LYF1999 force-pushed the yf/issue-100741 branch from f615844 to ca90121 Compare August 25, 2022 08:18

This comment has been minimized.

Sign in to view

LYF1999 force-pushed the yf/issue-100741 branch from ca90121 to d436357 Compare August 25, 2022 08:55

give some suggestion for returning anonymous enum

bac60db

LYF1999 force-pushed the yf/issue-100741 branch from d436357 to bac60db Compare August 25, 2022 09:19

estebank reviewed Sep 1, 2022

View reviewed changes

LYF1999 marked this pull request as draft September 22, 2022 07:30

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2022

nicopap mentioned this pull request Jan 30, 2023

Closure with a typed function argument and top level struct constructor break in nightly #107461

Closed

Dylan-DPC closed this May 15, 2023

Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2023

	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,
	}
	}

Uh oh!

Conversation

LYF1999 commented Aug 25, 2022

Uh oh!

LYF1999 commented Aug 25, 2022

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jan 26, 2023

Uh oh!

Noratrieb commented Mar 14, 2023

Uh oh!

LYF1999 commented Mar 14, 2023

Uh oh!

bors commented May 2, 2023

Uh oh!

Dylan-DPC commented May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants