Several issues with completing method bindings

[stephan]

this issue is spawned from #24 where completing individual elements like types within a method binding are discussed, whereas this issue is about creating full method specs using completion.

When completing the RHS of a method binding, several combinations caused problems. The following situations have to be supported:

• completion after "->", "=>" and "<-"
• completion after "<-" with or without callin modifier (before|replace|after)
• completion after any binding token where the LHS spec is either short or long (with signature)

Note that causes for these issues are scattered over

• Parser (performing error recovery and setting positions)
• CompletionEngine (initially generating proposals),
• CompletionAdaptor (creating rewrite-based proposals from string-based proposals) and
• ASTRewriteAnalyzer (performing rewrites).

[stephan]

Completing after "<-" has been broken by r17685 (see comment // explicit callin proposals not implemented.), when a unified completion was introduced to first select the base method and then choose the binding kind by linked-mode proposals (see #31).

This breakage is fixed in r18088 by introducing a new completion class CallinRHSCompletionProposal.

Note that the new proposal proposes all three callin modifiers independent of whether the role method has a callin modifier or not. While this can result in an illegal binding it might be quite convenient to first create the binding and then use another quickfix to add the callin modifier to the role method if so desired.

Since r18091 the modifier that is initially proposed is chosen such as to be legal wrt the bound role method.

[stephan]

Completing after "<-" with a callin modifier already typed in is fixed in r18089. (The commit wrongly mentions #25, should have been #24).

[stephan]

Correctly selecting short/long method spec for callin RHS has been implemented as part of r18088.

For callout the same is implemented in r18092.

All that together closes this issue.

[mosconi]

some issues with latest changes (on Windows): * using completion right after the "->" or "<-" should add a single space before RHS * using callin completion puts base method on the next line and adds "," at the end

Error: Failed to load processor image

No macro or processor named 'image' found

[mosconi]

screenshot here:

[mosconi]

It seems that the line break issue only appears, if the following line is not empty.

[stephan]

OK, I managed to reproduce both problems from the previous comments. Note that the callin-problem (line break etc.) only occurs if the next element is a callout binding with signature :-/

Mh, this might be interpreted as adding another base method to an existing callin binding, so

rm2 <- // completing here
void m() -> void m();

is actually parsed as

rm2 <- void m() // almost OK so far
-> m();         // trailing garbage

Then the new method spec wants to sit on the same line as its sibling and adds a ',' as the separator ...

Not sure, if the parser can possibly detect the programer's intention here..

Reopening.

[stephan]

The issue of a leading space for callout has been fixed in r18100, r18101. For callin it supposedly already worked before. If you have a counter example, please send it.

Technical comment: with the mentioned patch all completion proposals relating to generating (parts of) method bindings are now derived from a common super class MethodMappingCompletionProposal. This class is purely rewrite-based thus providing some consistency (in contrast to a previous implementation which used the rewriting only to create a string-based proposal).

[stephan]

The issue illustrated in the screenshot of comment 5 can indeed be fixed by making the parser smarter. r18104 fixes it for the following combination:

• signature-less partial method binding (callin or callout)
• next element is either a concrete method or a method binding with signatures.

Other combinations to follow soon.

Technical comment: the parser encodes method specs temporarily as MethodDeclaration. Thus during error recovery a MethodDeclaration is added to the current RecoveredMethodMapping. Using Scanner look-ahead this situation must distinguish method specs from real elements by looking for "{" (it's a method) or "->"/"<-" (it's a fresh new method mapping).

Problems are to be anticipated for abstract methods (have no "'{'") (use the abstract modifier!).

[stephan]

r18106 brings a number of new tests for various combinations discussed above. What's new: these tests cover the completion engine and the UI part including the rewriting process.

The same patch also includes fixes all currently known issues discussed here.

[stephan]

one more test similar to the situation of comment:7 has been added in r19167, r19168. The difference being: the subsequent declaration does not start with a keyword (void) but with an identifier (String).

New solution is to keep the ambiguously parsed method spec in the method mapping, and let RecoveredMethodMapping#updateSourceEndIfNecessary() detect overlap with the subsequent declaration and then replace the bogus method spec with an empty one.