Skip to content

fix: Don't panic on some weird code #19738

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

Merged
merged 1 commit into from
May 6, 2025

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented May 3, 2025

Fixes #19734.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2025
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

This seems a bit odd to me, handling it this way. I believe we should be bailing out earlier for a such a broken path already, that is a path solely consisting of a type anchor (<T as Trait>) which i believe is the issue here?.

Also I'd rather have this as a test in hir-ty itself imo (though I can understand if reproducing that without IDE tools might be tricky)

@Veykril Veykril 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 May 5, 2025
@ChayimFriedman2 ChayimFriedman2 force-pushed the weird-gats branch 2 times, most recently from 0615aa4 to 1db4e6a Compare May 5, 2025 21:04
@ChayimFriedman2
Copy link
Contributor Author

This seems a bit odd to me, handling it this way. I believe we should be bailing out earlier for a such a broken path already, that is a path solely consisting of a type anchor (<T as Trait>) which i believe is the issue here?.

Apparently we don't 🙂 We probably could, and I have no preference to handle it one way or another, but I don't know where we can handle this (parsing must produce some syntax tree anyway, and hir lowering doesn't know the path was malformed).

Also I'd rather have this as a test in hir-ty itself imo (though I can understand if reproducing that without IDE tools might be tricky)

Done.

@Veykril
Copy link
Member

Veykril commented May 6, 2025

Apparently we don't 🙂

Oh no I am aware, I meant that more in a manner of, we should be doing that instead (bad phrasing on my part).

@ChayimFriedman2
Copy link
Contributor Author

Do you have an idea how?

@Veykril
Copy link
Member

Veykril commented May 6, 2025

No, I am looking a bit at it right now but I think I am wrong either way. We do have to handle this here I think

@Veykril
Copy link
Member

Veykril commented May 6, 2025

Hmm, actually, we can just fail path lowering if we end up with a path whose last segment has has_self_type set I think, as that is always incorrect?

@Veykril
Copy link
Member

Veykril commented May 6, 2025

That is could you check (something along the lines of)

index 629d1f2ada..e664f83f6e 100644
--- a/crates/hir-def/src/expr_store/lower/path.rs
+++ b/crates/hir-def/src/expr_store/lower/path.rs
@@ -110,7 +110,7 @@
                 push_segment(&segment, &mut segments, Name::new_symbol_root(sym::Self_));
             }
             ast::PathSegmentKind::Type { type_ref, trait_ref } => {
-                debug_assert!(path.qualifier().is_none()); // this can only occur at the first segment
+                debug_assert!(path.qualifier().is_none() && type_anchor.is_none()); // this can only occur at the first segment

                 let self_type = collector.lower_type_ref(type_ref?, impl_trait_lower_fn);

@@ -235,6 +235,10 @@
     let mod_path = Interned::new(ModPath::from_segments(kind, segments));
     if type_anchor.is_none() && generic_args.is_empty() {
         return Some(Path::BarePath(mod_path));
+    } else if generic_args.last().and_then(Option::as_ref).is_some_and(|it| it.has_self_type) {
+        // `<T as Trait>` without following segment
+        // FIXME: Report error
+        return None;
     } else {
         return Some(Path::Normal(Box::new(NormalPath {
             type_anchor,

I think that would be a better solution here

@ChayimFriedman2
Copy link
Contributor Author

I hesitate a bit because maybe there is a valid way for this to happen, but I changed to that, except that I don't return None but only dismiss the generic args, which will cause us to infer them - a better user experience.

@Veykril Veykril added this pull request to the merge queue May 6, 2025
Merged via the queue into rust-lang:master with commit debaef8 May 6, 2025
14 checks passed
@ChayimFriedman2 ChayimFriedman2 deleted the weird-gats branch May 6, 2025 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when parsing invalid type
3 participants