Unknown pipe & local ref diagnostics #33454

alxhub · 2019-10-28T22:30:21Z

No description provided.

JoostK · 2019-10-29T21:23:52Z

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts

+import {TcbSourceResolver, absoluteSourceSpanToSourceLocation, makeTemplateDiagnostic} from './diagnostics';
+
+/**
+ * Collects reports on template diagnostics which are


Incomplete comment

Completed comment.

JoostK · 2019-10-29T21:27:49Z

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts

+    const mapping = this.resolver.getSourceMapping(id);
+    const value = ref.value.trim();
+
+    const errorMsg = `No directive found with exportAs '${value}'.`;


For future consideration: have an intuitive way of visualising the module scope/available directive exports to better indicate what needs to happen to resolve this diagnostic. Because it's a non-local issue, how to fix it is not straightforward (and one of the areas I would like to see Angular become better is in error reporting!)

I completely agree. One of the goals of having the centralized ErrorCode for Angular errors was to keep track of the different classes of errors and make it easier for us to set goals of improving them.

JoostK · 2019-10-29T21:31:08Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -797,6 +798,7 @@ class Scope {
      for (const child of node.children) {
        this.appendNode(child);
      }
+      this.checkReferencesOfNode(node);


I am surprised you took this approach, intertwining the reporting of missing refs with the generation of TCBs, instead of adding some diagnostic reporting facility to R3TargetBinder itself.

I considered this, actually. Part of the problem is that the way we map expressions back into the original template source depends a lot on the way we traverse the template. This doesn't matter for References which are a top level concept and have proper source spans, but for BindingPipes which are an AST node, the source mapping depends on the context and the R3TargetBinder doesn't currently deal with those concerns.

Additionally, it's easier within the TCB generator to conform to the checkTemplateBodies config option.

Ultimately we might refactor in this direction. Especially because the HTML-level template parsing errors are not currently surfaced via our "nice" diagnostics mechanism. We'll eventually want to port that over too.

Maybe it would make more sense to refactor the template traversal out of the TCB code, and make TCB generation a side effect as well. So the core template typechecking engine would walk through the template, and call into each "plugin" to do things like check reference validity, check the DOM schema, and cause TCBs to be emitted into the type check file.

JoostK · 2019-10-29T21:38:51Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -1025,7 +1036,11 @@ class TcbExpressionTranslator {
    } else if (binding instanceof TmplAstReference) {
      const target = this.tcb.boundTarget.getReferenceTarget(binding);
      if (target === null) {
-        throw new Error(`Unbound reference? ${binding.name}`);
+        // This reference is unbound.
+        this.tcb.oobRecorder.missingReferenceTarget(this.tcb.id, binding);


I am under the impression that reporting this both here and in checkReferencesOfNode is not ideal and may introduce the same diagnostic twice.

For instance:

<div dir #ref="nonexistent">{{ ref.value }}</div>

This will create two diagnostics with identical message but different spans, if I'm not mistaken:

<div dir #ref="nonexistent">{{ ref.value }}</div> ^~~~~~~~~~~~~~~~~~ ^~~

Both have the message No directive found with exportAs 'nonexistent'.. For the first diagnostics this message makes sense, for the second it does not.

Very good point! Fixed.

JoostK · 2019-10-29T21:48:42Z

packages/compiler/src/render3/view/t2_binder.ts

        if (dirTarget === null) {
-          // TODO(alxhub): Return an error value here that can be used for template validation.
-          throw new Error(`Assertion error: failed to find directive with exportAs: ${ref.value}`);
+          // No matching directive was found - this reference points to an unknown target.


There's a comment on L263 that mentions "One should exist.", this is now outdated.

Also, this is the place where I expected some error reporting mechanism to be used, instead of in the TCB generation code.

Good catch, fixed.

JoostK · 2019-10-29T21:50:23Z

packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts

@@ -55,6 +55,11 @@ export function toAbsoluteSpan(span: ParseSpan, sourceSpan: ParseSourceSpan): Ab
  return <AbsoluteSpan>{start: span.start + offset, end: span.end + offset};
 }

+export function absoluteSourceSpanToSourceLocation(


Hmm, maybe we should just decompose TcbSourceResolver.sourceLocationToSpan to receive two arguments instead of the SourceLocation bundle.

I'm all for refactoring this to clean it up :)

JoostK · 2019-10-29T21:52:03Z

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts

+    const location = absoluteSourceSpanToSourceLocation(id, absSpan);
+    const sourceSpan = this.resolver.sourceLocationToSpan(location);
+    if (sourceSpan === null) {
+      throw new Error(`Could not map location for error`);


Pretty cryptic error message ;)

JoostK · 2019-10-29T21:54:15Z

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts

@@ -20,12 +20,16 @@ export interface OutOfBandDiagnosticRecorder {
  readonly diagnostics: ts.Diagnostic[];

  missingReferenceTarget(id: string, ref: TmplAstReference): void;
+
+  missingPipe(id: string, ast: BindingPipe, sourceSpan: AbsoluteSourceSpan): void;


I now realize that id corresponds with the TCB id, not some pipe id which the function name suggests. These methods could use docs.

Renamed to templateId, and added docs.

JoostK · 2019-10-29T21:54:57Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -975,9 +977,14 @@ class TcbExpressionTranslator {
      return ts.createIdentifier('ctx');
    } else if (ast instanceof BindingPipe) {
      const expr = this.translate(ast.exp);
-      let pipe: ts.Expression;
+      let pipe: ts.Expression|null;
+      const nameAbsoluteSpan = toAbsoluteSpan(ast.nameSpan, this.sourceSpan);


Move nameAbsoluteSpan into the if (pipe === null) below

JoostK · 2019-10-29T21:58:54Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -34,9 +35,10 @@ import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsC
 */
 export function generateTypeCheckBlock(


I think we should make clear that this function has a side-effect when it comes to producing diagnostics. Having the out of bands parameter does indicate this somewhat, but it could be made more explicit. Come to think of it, it would be a good idea for future reference to document OutOfBandDiagnosticRecorder on why it exists and how it's used.

Done. I updated and expanded the comment to talk about the verifications which happen during generation.

As mentioned above, I think the "right way" to do this is to refactor the template traversal to be independent of everything, and hang various operations off of this central traversal, including both the verification diagnostics and TCB generation.

alxhub · 2019-10-30T22:05:36Z

Presubmit
Ivy Presubmit not needed: does not affect generated code, no new errors

JoostK

Just some more nits, otherwise LGTM

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

Previously the template binder would crash when encountering an unknown localref (# reference) such as `<div #ref="foo">` when no directive has `exportAs: "foo"`. With this commit, the compiler instead generates a template diagnostic error informing the user about the invalid reference.

Previously the compiler would crash if a pipe was encountered which did not match any pipe in the scope of a template. This commit introduces a new diagnostic error for unknown pipes instead.

Previously the template binder would crash when encountering an unknown localref (# reference) such as `<div #ref="foo">` when no directive has `exportAs: "foo"`. With this commit, the compiler instead generates a template diagnostic error informing the user about the invalid reference. PR Close #33454

Previously the compiler would crash if a pipe was encountered which did not match any pipe in the scope of a template. This commit introduces a new diagnostic error for unknown pipes instead. PR Close #33454

danielehrhardt · 2019-11-04T11:19:51Z

When do you plan to release it?

Previously the template binder would crash when encountering an unknown localref (# reference) such as `<div #ref="foo">` when no directive has `exportAs: "foo"`. With this commit, the compiler instead generates a template diagnostic error informing the user about the invalid reference. PR Close angular#33454

Previously the compiler would crash if a pipe was encountered which did not match any pipe in the scope of a template. This commit introduces a new diagnostic error for unknown pipes instead. PR Close angular#33454

Previously the template binder would crash when encountering an unknown localref (# reference) such as `<div #ref="foo">` when no directive has `exportAs: "foo"`. With this commit, the compiler instead generates a template diagnostic error informing the user about the invalid reference. PR Close angular#33454

Previously the compiler would crash if a pipe was encountered which did not match any pipe in the scope of a template. This commit introduces a new diagnostic error for unknown pipes instead. PR Close angular#33454

angular-automatic-lock-bot · 2019-12-05T16:02:11Z

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.}

alxhub requested a review from a team as a code owner October 28, 2019 22:30

alxhub requested a review from JoostK October 28, 2019 22:30

googlebot added the cla: yes label Oct 28, 2019

alxhub force-pushed the ngtsc/unknown-pipes-refs branch from 71391fd to fd267e8 Compare October 29, 2019 16:23

JoostK requested changes Oct 29, 2019

View reviewed changes

alxhub mentioned this pull request Oct 29, 2019

[Ivy] Error: Missing pipe: translate #32434

Closed

alxhub force-pushed the ngtsc/unknown-pipes-refs branch 2 times, most recently from c72ec40 to 65f8c43 Compare October 30, 2019 22:03

alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Oct 30, 2019

JoostK approved these changes Oct 30, 2019

View reviewed changes

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved

alxhub added 2 commits October 30, 2019 16:20

fix(ivy): don't crash on unknown pipe

8ee5c22

Previously the compiler would crash if a pipe was encountered which did not match any pipe in the scope of a template. This commit introduces a new diagnostic error for unknown pipes instead.

alxhub force-pushed the ngtsc/unknown-pipes-refs branch from 65f8c43 to 8ee5c22 Compare October 30, 2019 23:23

AndrewKushnir added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Oct 31, 2019

atscott closed this in 9db59d0 Oct 31, 2019

This was referenced Nov 5, 2019

Declaring a pipe in two modules does not produce a diagnostic [Importing pipes in sub modules does not compile] #33586

Closed

Ivy - exportAs - failed to find #33628

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unknown pipe & local ref diagnostics #33454

Unknown pipe & local ref diagnostics #33454

alxhub commented Oct 28, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019 •

edited

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

JoostK Oct 29, 2019

alxhub Oct 30, 2019

alxhub commented Oct 30, 2019

JoostK left a comment

danielehrhardt commented Nov 4, 2019

angular-automatic-lock-bot bot commented Dec 5, 2019

		@@ -34,9 +35,10 @@ import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsC
		*/
		export function generateTypeCheckBlock(

Navigation Menu

Unknown pipe & local ref diagnostics #33454

Unknown pipe & local ref diagnostics #33454

Conversation

alxhub commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub Oct 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub commented Oct 30, 2019

JoostK left a comment

Choose a reason for hiding this comment

danielehrhardt commented Nov 4, 2019

angular-automatic-lock-bot bot commented Dec 5, 2019

alxhub Oct 30, 2019 •

edited