Methods are not new identifier definition locations

[DanielRosenwasser]

let x = {
  /**/
};

Type in a( and we'll auto-complete the property. That shouldn't happen.

[andrewbranch]

addEventListener is a global, so it is possible that you wanted to write

let x = {
  addEventListener
}

It was added in #41539. I argued that you probably don’t ever want to do this with globals, but didn’t realize that ( was a commit character (IMO, way too many things are commit characters), so I left it up to dissenters, and none showed up:

Edit: everything else I said was based on a misunderstanding of what “new identifier location” was supposed to do

[andrewbranch]

@DanielRosenwasser I bring this up because I think your title for this issue is too strong of an assertion. That is a new identifier location, or else we have to revert #41539, which fixes an issue that you called a bug 😅. I can definitely get behind removing globals from the list here, but if you were in a file with a ton of locals, and had the intention to write a non-contextulaly-typed object literal with bespoke member names, you could hit the same frustration that typing ( gives you a bad completion.

[DanielRosenwasser]

There's 2 things:

1. Outside variables should (often?) be valid suggested shorthand property completions.
2. Property completions should be considered potentially new identifier locations (e.g. see uses of isNewIdentifierLocation and isNewIdentifierDefinitionLocation from what I recall)

The latter should permit people to write out a new name without a commit character committing to an arbitrary global or local.

[DanielRosenwasser]

That "often" has more to do with a contextual type. If you have a contextual type, you probably just want to provide those completions.

[DanielRosenwasser]

It sounds like there's another bug here which is that isNewIdentifierDefinitionLocation is very poorly documented. 😅

The idea is that there's 3 states I have in mind

1. Always give completions.
2. Never give completions (isNewIdentifierLocation)
3. Give completions, but don't auto-complete on commit characters (isNewIdentifierDefinitionLocation() -> true)

And I'm saying we want the third one.

[andrewbranch]

Got it, I thought you were talking about (2) but you were talking about (3).

[andrewbranch]

If an object literal has a contextual type, are its members still new identifier definition locations? That function currently works off syntax alone.

Edit: actually, it pretty much Just Works—isNewIdentifierDefinitionLocation only runs when we collect locals/globals, which we don’t do under a contextual type. The only questionable result is

function f2<T extends { xyz: number }>(x: T) {}
f2({ x/*2*/ });

Here, we offer exactly one completion, xyz, but isNewIdentifierLocation is set to false. Since the contextual type is only a constraint, it really is a new identifier definition location, but I think it doesn’t matter much since there are no extraneous completions.

[DanielRosenwasser]

Yeah that doesn't seem too bad I suppose.