fix(language-service): two-way binding completion should not remove t…

[ivanwonder]

…he trailing quote

For two-way binding syntax, the Angular compiler will append the =$event to the expression of
BoundEvent.

angular/packages/compiler/src/render3/r3_template_transform.ts

Line 493 in e0ac614

`${name}Change`, `${expression} =$event`, /* isAssignmentEvent */ true, sourceSpan,

For example, the [(model)]="title.¦", the expression of BoundEvent will be converted to
title.¦ =$event.
--------^------ this blank will be included in the replacementSpan of completion item.
When the user selects an item, the trailing quote will be removed.

Now the paths include BoundAttribute and BoundEvent for the two-way binding syntax. So the
BoundEvent should be removed from the paths and use the BoundAttribute instead.

Fixes angular/vscode-ng-language-service#1626

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[atscott]

This is a regression caused by db6cf7e which is creating an inaccurate source span for the synthetic bound event used for the completion.

@JoostK Can we change the implementation to just remove the trailing ! if the expression ends with it? Doing that would simplify the implementation, since the LHS wouldn't have the ! at the end. I know this would still not have accurate source spans, but that wouldn't be as much of a problem here since there wouldn't be completions for something that just ends with a !.

i.e.

   if (expression.endsWith('!')) {
      expression = expression.slice(0, -1);
    }
    this.bindingParser.parseEvent(
        `${name}Change`, `${expression}=$event`, /* isAssignmentEvent (no longer needed) */ true, sourceSpan,
        valueSpan || sourceSpan, targetMatchableAttrs, events, keySpan);

[JoostK]

Oh, interesting; that is definitely an unexpected side-effect of db6cf7e!

Can we change the implementation to just remove the trailing ! if the expression ends with it?

I am a bit wary of doing that, as it would again be susceptible to unexpected parses in combination with the = sign. The only valid expression that I can think of that would cause trouble is when using double (or repeated really) non-null assertions; [(ngModel)]="value!!", which admittedly is easy to account for with your suggestion as well. Then there's things like [(ngModel)]="input>" which results in a valid parse tree input >= $event for the output binding, but this is caught as an error for the input binding so unlikely to become a real problem.

Overall the current parsing approach is awful and would benefit from an overhaul, also to fix the source span issues.

[atscott]

In that case, I would propose updating the template visitor logic to not create this situation in the first place rather than filtering things out after the fact. This bit of code could be added to the visitElementOrTemplate after this.visitAll(element.inputs);.

    // We allow the path to contain both the `t.BoundAttribute` and `t.BoundEvent` for two-way
    // bindings but do not want the path to contain both the `t.BoundAttribute` with its
    // children when the position is in the value span because we would then logically create a path
    // that also contains the `PropertyWrite` from the `t.BoundEvent`. This early return condition
    // ensures we target just `t.BoundAttribute` for this case and exclude `t.BoundEvent` children.
    if (this.path[this.path.length - 1] !== element &&
        !(this.path[this.path.length - 1] instanceof t.BoundAttribute)) {
      return;
    }

There should also be a test in the template_target_spec for this, not just in the completions_spec.ts.

@JoostK Had some feelings about changing the implementation of the visitor to "extend the return type of TemplateTargetVisitor.visitTemplate to be the path to BoundAttribute, together with a (nullable) pointer to a BoundEvent" so we aren't breaking the mental model of the path by having the BoundEvent look like a child of the BoundAttribute when there's a two way binding. This is a bit larger of a refactor though.

[ivanwonder]

In that case, I would propose updating the template visitor logic to not create this situation in the first place rather than filtering things out after the fact. This bit of code could be added to the visitElementOrTemplate after this.visitAll(element.inputs);.

The BoundEvent is still needed in the path.

angular/packages/language-service/src/definitions.ts

Lines 288 to 310 in a6ab3f7

	private getDefinitionMetaAtPosition({template, component}: TemplateInfo, position: number):
	DefinitionMeta[]|undefined {
	const target = getTargetAtPosition(template, position);
	if (target === null) {
	return undefined;
	}
	const {context, parent} = target;
	const nodes =
	context.kind === TargetNodeKind.TwoWayBindingContext ? context.nodes : [context.node];
	const definitionMetas: DefinitionMeta[] = [];
	for (const node of nodes) {
	const symbol = this.compiler.getTemplateTypeChecker().getSymbolOfNode(node, component);
	if (symbol === null) {
	continue;
	}
	definitionMetas.push({node, parent, symbol});
	}
	return definitionMetas.length > 0 ? definitionMetas : undefined;
	}
	}

[atscott]

The BoundEvent is still needed in the path.

Only for the case where the previous value in the path is a BoundAttribute. Otherwise it's not identified as a two way binding anyways.

[ivanwonder]

Oh, I got it, but I think this is easy to understand for me

    const length = this.path.length;
    this.visitAll(element.inputs);
    const noMatchedInputs = this.path.length === length;
    const matchedBoundAttribute =
        this.path.length - length === 1 && this.path[length] instanceof t.BoundAttribute;
    if (noMatchedInputs || matchedBoundAttribute) {
      this.visitAll(element.outputs);
    }

visit the event only when there are no matched inputs or there only has one BoundAttribute.

[atscott]

@ivanwonder Right, that's pretty close to the same thing. The issue I have is that it really can be an early exit from the rest of the visit calls. If you visit the inputs and match anything, the rest of the visit logic doesn't need to happen (aside from possibly the BoundEvent for two way binding). There's nothing possibly more specific. So it shouldn't be just if (condition) visitOutputs but if (condition) early exit because we know we have the most specific match possible

Edit: I think the distinction might really not matter too much in the end. The rest of the visitor logic would just exit immediately since the position would be outside the spans. But I do think it's easier to understand if (we know we're done) return; instead of if (something) do more work.

[ngbot]

I see that you just added the action: merge label, but the following checks are still failing:
     status "ci/circleci: legacy-unit-tests-saucelabs" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

[atscott]

merge assistance: the failing test is unrelated

[dylhunn]

This PR was merged into the repository by commit f57e46c.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}