Bug 569935 - ContentAssist autocomplete adds incorrect variables as argument suggestions using type matching not name matching.
Summary: ContentAssist autocomplete adds incorrect variables as argument suggestions u...
Status: NEW
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 4.18   Edit
Hardware: All All
: P3 normal with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: JDT-Text-Inbox CLA
QA Contact: Roland Grunberg CLA
URL:
Whiteboard: stalebug
Keywords:
: 569934 (view as bug list)
Depends on:
Blocks: 423642 433121
  Show dependency tree
 
Reported: 2020-12-27 19:18 EST by Garret Wilson CLA
Modified: 2023-03-16 14:11 EDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Garret Wilson CLA 2020-12-27 19:18:24 EST
I just upgraded to Eclipse Enterprise 2020-12 (4.18.0). Normally when ContentAssist autocompletes a method, it automatically inserts the parameter names from the method signature. For example let's say I have this method in `FooBar`:

    public void doSomething(String foo, String bar);

Until Eclipse 2020-12, if I used ContentAssist to add `FooBar.doSomething()`, Eclipse would add the parameter names from the signature, like this:

    fooBar.doSomething(foo, bar);

That was OK. If I happened to have variables `foo` and `bar` in that scope, there was a good chance they were the correct arguments (because they had the same name). If there were no such variables with those names, I would see a syntax error and know to correct it.

Now in Eclipse 2020-12, however, Eclipse goes and arbitrarily adds variables that have the same _type_, regardless of the name. It is so bad that Eclipse may add the same variable as multiple arguments! Thus instead of `fooBar.doSomething(foo, bar)`, I get this:


    fooBar.doSomething(subtleBug, subtleBug);

Just because `subtleBug` has the same type as the two method parameters, there is absolutely no indication that `subtleBug` is the correct argument for this method. In fact it almost certainly is _not_ the correct argument if it is used multiple times, so Eclipse has virtually guaranteed that it is supplying me the wrong variable.

And to make this worse, if Eclipse guesses wrong (which is very likely as I explained), there is a greater chance that I will never notice it because there is no syntax error, seeing that Eclipse chose an actual variable in scope (even though it was very likely the wrong variable).

ContentAssist has taken some huge steps backwards in Eclipse 2020-12 (see also Bug 569934), and this one is dangerous on top of it. I am almost certainly going to downgrade to Eclipse 2020-09.
Comment 1 Garret Wilson CLA 2020-12-27 19:26:06 EST
And to make matters worse, because I don't see what the original parameter names are from the method signature (all I see is `subtleBug` and `subtleBug` in the previous example I gave), I have no idea what the parameters _should_ be, so I have to go drill down to the actual method in another file, wasting whatever time I initially saved using ContentAssist and completely defeating its purpose.
Comment 2 Garret Wilson CLA 2020-12-28 10:37:18 EST
This is really bad; it has already introduced bugs even though I've been using Eclipse 2020-12 only a few days.

This is particularly bad when I am adding convenience methods with different parameter counts. For example let's say I have the following method:

    public void doSomething(String foo, String bar, String comment);


I want to add a convenience method that does exactly the same thing but without a comment. So I add this method right above it:

    public void doSomething(String foo, String bar) {
      doSomething|
    }

Note where my cursor is after `doSomething`. I want to use autocomplete to add a reference to the `doSomething()` with three parameters. So in Eclipse 2020-09 (and everything before, for years on end) I would press `Ctrl+Space` and get this:

    public void doSomething(String foo, String bar) {
      doSomething(foo, bar, comment);
    }

Note that the `foo` and the `bar` come _from the method signature_. Eclipse 2020-09 would not try to guess what arguments to use based on variables in scope. This would show a syntax error(good!), so I would simply change `doSomething(foo, bar, comment)` to `doSomething(foo, bar, null)` and be on my way.

Now, however, Eclipse 2020-12 completely ignores the parameter names from the signature, looks around and find some arbitrary variable with the same time, and gives me this:


    public void doSomething(String foo, String bar) {
      doSomething(unrelated, unrelated, unrelated);
    }

None of this is even close to being correct. But even worse, there is no syntax error. So maybe I'll remember to change the last `unrelated` to `null`, because that is the comment parameter. (But how do I even remember that without going back to the method I'm calling?) But even with a great memory and changing the last `unrelated` to `null`, it's hard to remember that the other `unrelated` variables are completely wrong, because they compile. This sort of error has already happened to me.
Comment 3 Garret Wilson CLA 2020-12-28 11:14:21 EST
Here's another example: I'm writing a convenience method to work with the XML DOM:

    /**
     * Adds a new attribute. If an attribute with the same local name and namespace URI is already present on the element, its prefix will be changed to be the
     * prefix part of the qualified name, and its value will be updated.
     * @implSpec This implementation delegates to {@link Element#setAttributeNS(String, String, String)}.
     * @param nsQualifiedName The namespace URI and qualified name of the attribute to create or alter.
     * @param value The value to set.
     * @throws DOMException if there was a DOM error creating or altering the attribute.
     */
    public static void setAttribute(@Nonnull final Element element, @Nonnull final NsQualifiedName elementName, @Nonnull final String value) throws DOMException {
      element.setAttributeNS|
    }

Note where my cursor is after `element.setAttributeNS`. In Eclipse 2020-09 and before, autocomplete would give me the following, after it looked at the API for `org.w3c.dom.Element.setAttributeNS()`:

      element.setAttributeNS(namespaceURI, qualifiedName, value);

That would great! `value` would be correct, but I would see that `namespaceURI` and `qualifiedName` were underlined in read and needed updating. But I knew their names, so I knew what they did. So I would quickly change them:

      element.setAttributeNS(elementName.getNamespaceString(), elementName.getQualifiedName(), value);

Now however Eclipse 2020-12 gives me this:

		element.setAttributeNS(value, value, value);

Completely useless. There is hardly any chance that `value` would have been the correct argument to be repeated for all the parameters. Even worse, I have no idea what the original parameters were without pulling up the source code to `org.w3c.dom.Element.setAttributeNS()`. And lastly if I'm not extra vigilant, I might not even notice there is a problem because this compiles. Or I might miss one of the `value` variables I need to change.

I cannot work with this. I'm reverting to 2020-09.
Comment 4 Andrey Loskutov CLA 2020-12-28 11:23:15 EST
I guess you don't need to switch back, you can simply change the preference, see https://www.eclipse.org/eclipse/news/4.18/jdt.php#param-best-guessed. See bug 433121.
Comment 5 Garret Wilson CLA 2020-12-28 11:23:50 EST
Wow, autocomplete is even pulling out arbitrary static import constants! Thus if I have the following:

    import static com.example.HundredsOfConstants.*;


Then when I use autocomplete on:

    public void doSomething(String foo, int bar);

I get some arbitrary constants!!!

    doSomething(SOME_RANDOM_THING, SOMETHING_ELSE);

I trust you get the idea of how counterproductive and error-prone this is.
Comment 6 Garret Wilson CLA 2020-12-28 11:29:47 EST
(In reply to Andrey Loskutov from comment #4)
> I guess you don't need to switch back, you can simply change the preference,
> see https://www.eclipse.org/eclipse/news/4.18/jdt.php#param-best-guessed.

Thank you for the tip, Andrey. But is there also an option to make it Content Assist stop deleting subsequent characters as I reported in Bug 569934? This is another huge time waster.
Comment 7 Garret Wilson CLA 2020-12-28 11:33:14 EST
(In reply to Andrey Loskutov from comment #4)
> I guess you don't need to switch back, you can simply change the preference,

Why didn't this preference default to "the way it worked before"? That's just standard common-sense best practice for adding new features, unless something was broken about the way it worked before. That way users can opt-in to the new behavior only if they want to; otherwise, it works the way they like it. Why do product makers keep breaking this rule and causing users grief?
Comment 8 Garret Wilson CLA 2020-12-28 11:45:39 EST
(In reply to Garret Wilson from comment #6)
> (In reply to Andrey Loskutov from comment #4)
> > I guess you don't need to switch back, you can simply change the preference,
> > see https://www.eclipse.org/eclipse/news/4.18/jdt.php#param-best-guessed.
> 
> Thank you for the tip, Andrey. But is there also an option to make it
> Content Assist stop deleting subsequent characters as I reported in Bug
> 569934? This is another huge time waster.

Wow, yes, I see that this is an option as well:

https://www.eclipse.org/eclipse/news/4.18/jdt.php#completion-overwrites

Wow. "Add an option for a significant change in behavior, but default the setting to the current behavior." It would have been so simple. Why, why, why?
Comment 9 Andrey Loskutov CLA 2020-12-28 11:50:04 EST
(In reply to Garret Wilson from comment #6)
> (In reply to Andrey Loskutov from comment #4)
> > I guess you don't need to switch back, you can simply change the preference,
> > see https://www.eclipse.org/eclipse/news/4.18/jdt.php#param-best-guessed.
> 
> Thank you for the tip, Andrey. But is there also an option to make it
> Content Assist stop deleting subsequent characters as I reported in Bug
> 569934? This is another huge time waster.

For the record: this is after bug 423642 preference change.
Comment 10 Wim Jongman CLA 2020-12-28 12:47:36 EST
Garret, any change will always bring some level of frustration to someone. We make these changes because the team thinks that this is the desired out-of-the-box behavior.

However, thank you for filing this; it will help us consider future changes.
Comment 11 Garret Wilson CLA 2020-12-28 12:59:03 EST
(In reply to Wim Jongman from comment #10)
> Garret, any change will always bring some level of frustration to someone.
> We make these changes because the team thinks that this is the desired
> out-of-the-box behavior.
> 
> However, thank you for filing this; it will help us consider future changes.


Thank you for your response, Wim.

I understand what you are saying. But note that you said "out-of-the-box" behavior. That is very different than "changing the behavior of my existing workspace which I've been using for well over a decade". If someone has a configured workspace that works the way they want it to, it is a bad idea to suddenly change that on them. Otherwise the user will never know if upgrading will suddenly make things work differently with their existing workspace, so the user will be inclined not to upgrade.

If you want to make this functionality have a different initial setting in a completely new workspace, that's a different story altogether. That's what you're talking about. But you give existing, loyal users a bad taste in their mouth when you _change_ what they have their workspace to set up to work exactly how they want it to. Maybe existing users will like the new functionality better. But you give them the option to opt-in. You don't force it on them.

But you may say, "We think this option is so great that everyone will like it; how will the users know about these new options we have?" Well you just need to do a better job on advertising these features. The Git for Windows installer, for example, places new features (e.g. native support for symbolic links) in a panel in the installer wizard so that users may turn on these new feature _if they want to_ during installation. Or maybe you open a new Eclipse window when a new version is first run offering the user the option to change preferences for an existing workspace.

But you don't just go muck around with the behavior of existing workspaces unless you want to really want to ruin a user's day. Or several days.
Comment 12 Garret Wilson CLA 2020-12-28 13:14:09 EST
And lastly, if the new functionality hasn't been thoroughly tested in real life, then you most definitely don't want to force it on existing users, even if you think it is the best thing since sliced bread, because your great idea may have huge problems in real life. (A similar thing happened in VS Code with the bungled initial rollout of the "mirror cursor on matching tag" functionality. See https://github.com/microsoft/vscode/issues/87737 .)

In this case, it sounds like a great idea for Eclipse to guess the variables from the current scope rather than just inserting names. If Eclipse could think, it could program for us! Unfortunately the feature as currently implemented has a couple of huge flaws as I explained above:

* Duplicated variables for common types such as `String` will almost _always_ be wrong. (When is the last time you called a method with three string parameters, and used the same variable as the arguments?)
* If a class statically imports 100 constants, how could Eclipse possibly choose the correct one? Having Eclipse arbitrarily choose one of these constants doesn't help anyone.

Maybe after six months of experimentation, getting user feedback, and improving the intelligence, maybe you could a way to choose variables when Eclipse has high confidence, falling back to parameter names when there is no obvious choice. But you only find this out after lots of real life usage.

In summary: If you have a change in existing behavior that has not been tested extensively in real life and received overwhelming positive user feedback, don't force it on existing users. Even then, don't force it on existing users. Just don't. Thank you.
Comment 13 Noopur Gupta CLA 2021-01-04 03:02:17 EST
*** Bug 569934 has been marked as a duplicate of this bug. ***
Comment 14 Noopur Gupta CLA 2021-01-04 03:10:59 EST
Garret, thanks for your feedback. This should be taken into account while changing defaults going forward. 

As mentioned in previous comments, please change the preferences in your workspace to resolve the differences or report bugs for issues found with the new default behavior so that those can be fixed.
Comment 15 Eclipse Genie CLA 2023-03-16 14:11:19 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.

--
The automated Eclipse Genie.