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

INT: Support "Create associated function" for path Self::func() #9335

Merged
merged 1 commit into from Sep 28, 2022

Conversation

dima74
Copy link
Member

@dima74 dima74 commented Sep 9, 2022

changelog: Support Create associated function intention for path Self::func()

@dima74 dima74 added the fix Pull requests that fix some bug(s) label Sep 9, 2022
@dima74 dima74 requested a review from Kobzol September 9, 2022 21:16
@dima74 dima74 added this to In Progress in To test via automation Sep 9, 2022
@Undin Undin added feature and removed fix Pull requests that fix some bug(s) labels Sep 10, 2022
Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Could it also work with <this struct>::Foo? For example:

struct Foo;

impl Foo {
    fn foo() {
        Foo::bar();
    }
    // create fn bar() {}
}

Maybe we could just always detect/find an existing impl block and use it if it's available? That seems like it could unify the "OtherType::foo()", "::foo()" and "Self::foo()" use-cases.


is RsStructOrEnumItemElement -> {
text = "Create associated function `${target.name}::$name`"
Context.AssociatedFunction(functionCall, name, target.containingMod, target, existingImpl = null)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like in the current usage, existingImpl and item are exclusive? It could be modeled by some enum, but it's probably overkill.

@dima74
Copy link
Member Author

dima74 commented Sep 12, 2022

Could it also work with <this struct>::Foo?

It already works but creates new impl. As you said, it would be nice to find an existing impl, and I plan to do it next. Btw for Self::foo() we can support the intention even if Self is not ADT, since Self resolves to an impl

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Ok, in that case LGTM :)

Copy link
Member

@artemmukhin artemmukhin left a comment

Choose a reason for hiding this comment

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

LGTM. Regarding this intention in general, I noticed that it does not reuse existing impl blocks when generating new associated functions or methods, e.g.:

struct  S;

impl S {
    fn foo() {}
}

// new `impl S` block for `bar` will be added instead of reusing the previous one

fn foo() {
    S::bar/*caret*/(1, 2);
}

IMO it would be nice if the intention could avoid generating unnecessary impl blocks in such cases, for example when there is only one existing impl block located in the same file. @dima74 WDYT?

@dima74
Copy link
Member Author

dima74 commented Sep 28, 2022

IMO it would be nice if the intention could avoid generating unnecessary impl blocks in such cases, for example when there is only one existing impl block located in the same file. @dima74 WDYT?

Yeah I plan to do this next

@dima74
Copy link
Member Author

dima74 commented Sep 28, 2022

bors r=ortem

@bors
Copy link
Contributor

bors bot commented Sep 28, 2022

Build succeeded:

@bors bors bot merged commit 0243996 into master Sep 28, 2022
To test automation moved this from In Progress to Test Sep 28, 2022
@bors bors bot deleted the diralik/create-associated-function-Self branch September 28, 2022 22:17
@github-actions github-actions bot added this to the v180 milestone Sep 28, 2022
@neonaot neonaot self-assigned this Oct 3, 2022
@neonaot
Copy link
Member

neonaot commented Oct 5, 2022

Judging by the tests, this is the expected behavior, but I opened the issue before looking at the tests :D

#9466

@neonaot neonaot moved this from Test to Done in To test Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
To test
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants