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

Code assist does not add necessary "owner" qualifier when accepting owner property suggestions within 'with' closure #364

Closed
mauromol opened this issue Nov 2, 2017 · 9 comments
Assignees
Milestone

Comments

@mauromol
Copy link

mauromol commented Nov 2, 2017

Consider the following:

package test

class Test {
  static class Foo {
	  String someString = 'foo'
  }
  
  static class Bar {
	  String someString = 'bar'
	  Foo foo = new Foo()
	  
	  void doSomething() {
		  foo.with {
			  println "someString inside with closure = $someString"
			  println "someString of owner inside with closure = $owner.someString"
			  someS|
		  }
	  }
  }
  
  static main(args) {
	  Bar b = new Bar()
	  println "b.someString = $b.someString"
	  println "b.foo.someString = $b.foo.someString"
	  b.doSomething()
  }
}

Invoke code assist at "|": Greclipse correctly suggests both Foo.someString and Bar.someString, but if you choose any of those, it completes with just someString.
However, if I choose Bar.someString, I would expect code assist to change/complete my word as owner.someString, as this is the right way (AFAIK) to actually reference Bar.someString from there.

@eric-milles
Copy link
Member

The owner proposals come from this block in StatementAndExpressionCompletionProcessor:

            if (context.location == ContentAssistLocation.STATEMENT) {
                ClassNode closureThis = requestor.currentScope.getThis();
                if (closureThis != null && !closureThis.equals(completionType)) {
                    // inside of a closure; must also add content assist for this (previously did the delegate)
                    proposalCreatorLoop(context, requestor, closureThis, isStatic, groovyProposals, creators, true);
                }

One thing that is not known until runtime is the Closure's resolve strategy. It defaults to OWNER_FIRST, but could be DELEGATE_FIRST, DELEGATE_ONLY, OWNER_ONLY or TO_SELF.

@eric-milles
Copy link
Member

Looks like it can be determined by looking at the method declaration in some cases:

    public static <T,U> T with(
            @DelegatesTo.Target("self") U self,
            @DelegatesTo(value=DelegatesTo.Target.class,
                    target="self",
                    strategy=Closure.DELEGATE_FIRST)
            @ClosureParams(FirstParam.class)
            Closure<T> closure)

@mauromol
Copy link
Author

mauromol commented Nov 2, 2017

Yes, I think some way should exist, at least for API methods, because otherwise I can't imagine how @CompileStatic could work...

@mauromol
Copy link
Author

mauromol commented Nov 7, 2017

I see your commit of 4 days ago is referencing this bug too, but with 2.9.2.xx-201711070247-e46 code assist is still just completing to someString whichever choice I make.

@eric-milles
Copy link
Member

The commit was just a step towards identifying delegate and owner correctly in this situation.

@eric-milles eric-milles added this to the v3.0.0 milestone Jan 9, 2018
@eric-milles eric-milles modified the milestones: v3.0.0, v3.1.0 Jun 15, 2018
@eric-milles eric-milles modified the milestones: v3.1.0, v3.2.0 Aug 18, 2018
@eric-milles
Copy link
Member

Proposal de-duplication changes for #735 have affected this use case. The proposals from both Foo and Bar will need to be offered again before the missing qualifier can be sorted out.

@eric-milles
Copy link
Member

Ready to test

@mauromol
Copy link
Author

With 3.2.0.xx-201811292113-e48 the mentioned use case works, although I would expect Foo.someString to be proposed before Bar.someString in this context.

Also, neither someString nor owner.someString are found when searching for references or invoking call hierarchy on Foo.someString and Bar.someString respectively. Should I open a new issue for this?

@eric-milles
Copy link
Member

Please open as separate issues. If qualifier is added when required, this issue is completed.

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

No branches or pull requests

2 participants