Proof of Concept: add #[defines] attribute and require it for all type-alias-impl-trait sites that register a hidden type
#128440
+2,066
−1,918
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ng from the signature to the list of opaques that are being defined
statics still need work
oli-obk
commented
Jul 31, 2024
Comment on lines
-250
to
+232
| self.tcx.dcx().emit_err(TaitForwardCompat2 { | ||
| let guar = self.tcx.dcx().emit_err(TaitForwardCompat2 { | ||
| span: self | ||
| .tcx | ||
| .def_ident_span(item_def_id) | ||
| .unwrap_or_else(|| self.tcx.def_span(item_def_id)), | ||
| opaque_type_span: self.tcx.def_span(self.def_id), | ||
| opaque_type: self.tcx.def_path_str(self.def_id), | ||
| }); | ||
| // TODO: land this change as a separate PR on master, it works on its own. | ||
| // Avoid "opaque type not constrained" errors on the opaque itself. | ||
| self.found = Some(ty::OpaqueHiddenType { | ||
| span: DUMMY_SP, | ||
| ty: Ty::new_error(self.tcx, guar), | ||
| }); |
There was a problem hiding this comment.
This change should be a fairly trivial PR on its own (just removes some redundant errors)
oli-obk
added
the
S-waiting-on-author
|
The job Click to see the possible cause of the failure (guessed by this bot) |
petrochenkov
added
S-waiting-on-review
3 tasks
|
☔ The latest upstream changes (presumably #128481) made this pull request unmergeable. Please resolve the merge conflicts. |
| } | ||
| MetaItemKind::NameValue(_) => vec![], | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this function ever used?
The ids used to identify defines paths are created with next_node_id in resolver, so they do not match ids kept in meta-item paths (if such ids are filled to non-dummy values at all).
| // TODO: error reporting and tests that go through these code paths | ||
| PathResult::Module(_) => todo!(), | ||
| PathResult::NonModule(partial_res) => { | ||
| let id = self.r.next_node_id(); |
There was a problem hiding this comment.
Are these ids necessary?
Probably simpler to push the resolutions straight to self.r.defines.entry(attr.id), without doing record_partial_res.
| match self.resolve_path( | ||
| &Segment::from_path(&item.path), | ||
| Some(Namespace::TypeNS), | ||
| None, |
There was a problem hiding this comment.
This is not a speculative resolution, so there should be Some here, but the real fix would be using smart_resolve_path instead of resolve_path above.
| fn check_defines(&self, attr: &Attribute, span: Span, target: Target) -> bool { | ||
| match target { | ||
| Target::Fn | ||
| | Target::Closure |
There was a problem hiding this comment.
The closure case is not processed in resolve.
|
Simple paths in attributes are not as bad as rust-lang/rfcs#3659, but still sort of meh, but probably acceptable. We don't currently have any resolved paths (staying after macro expansion) in attributes in the proper language. |
petrochenkov
added
S-waiting-on-author
|
Also, |
traviscross
added
the
F-type_alias_impl_trait
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is not meant to be merged (I will not be pushing changes in the next 4 months), but serves as a starting point for anyone wanting to make progress on this. It should hopefully be simple to rebase and not require you to edit 200+ tests, as these edits are done here and you can just cherry pick them out (no need for attribution, just squash as you want)
I left TODOs for everything that I think needs to be done.
#[defines()]), the latter opting out of impl-trait-in-assoc types automatic opaque collection in the signature (or in general, any#[defines(SomeOhterTait)]attribute behaving as an override for the default collection, too).An interesting follow-up could also be to automatically fill in RPITs into that list in ast lowering, so that
opaque_types_defined_bydoesn't need to walk for RPITs anymore.@petrochenkov can you have a look at the resolver changes so that anyone picking this up will know what to change?