5

Simple fix for make::impl_trait by alibektas · Pull Request #14621 · rust-lang/r...

 1 year ago
source link: https://github.com/rust-lang/rust-analyzer/pull/14621
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Conversation

Contributor

This is my first PR in this project. I made this PR because I needed this function to work properly for the main PR I am working on (#14386). This is a small amendment to what it was before. We still need to improve this in order for it to fully comply with its syntactic definition as stated here.

rustbot

added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label

Apr 21, 2023

Member

@Veykril Veykril

left a comment

The PR itself is still an improvement overall though, so let's merge it for now

@@ -184,8 +184,11 @@ pub fn impl_trait(

ty: ast::Path,

ty_params: Option<ast::GenericParamList>,

) -> ast::Impl {

let ty_params = ty_params.map_or_else(String::new, |params| params.to_string());

ast_from_text(&format!("impl{ty_params} {trait_} for {ty}{ty_params} {{}}"))

// TODO : If this function is now correct we can also change `impl_` accordingly`

impl_ as the comment states should take ty_args: Option<ast::GenericArgList>, not ty_params: Option<ast::GenericParamList>. Likewise it would be better if we change impl_trait to the following signature:

pub fn impl_trait(
    trait_: ast::Path,
    ty: ast::Path,
    ty_params: Option<ast::GenericParamList>,
    ty_args: Option<ast::GenericArgList>,
) -> ast::Impl {
alibektas reacted with thumbs up emoji

Member

@bors r+

Collaborator

pushpin Commit 4601331 has been approved by Veykril

It is now in the queue for this repository.

Collaborator

hourglass Testing commit 4601331 with merge af3b6a0...

Collaborator

sunny Test successful - checks-actions
Approved by: Veykril
Pushing af3b6a0 to master...

bors

merged commit af3b6a0 into

rust-lang:master

Apr 21, 2023

10 checks passed

Member

changelog internal (first contribution) support type parameters in make::impl_trait

Contributor

Author

After giving some thought I think I will fully implement the impl_trait syntax following the Rust Reference syntax ( see above for the goto link ). Should I make a separate PR for this or is it ok to keep using this PR ?

Member

This PR was already merged so you can't reuse (you can reuse the branch and open a new PR from it if you mean that)
alibektas reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Veykril

Veykril approved these changes
Assignees

No one assigned

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

None yet

5 participants

report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK