-
Notifications
You must be signed in to change notification settings - Fork 25.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(compiler): add name spans for property reads and method calls #36826
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
086d611
to
0bfecc7
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I do have some comments on the chosen source spans in the template type checker.
In general, I think our AST nodes could be cleaned up a lot if we were to have something like Identifier
, similar to the TypeScript AST. That seems like a large project by itself though, so it's not necessary for this PR. It may also be problematic to push that forward, as tools that depend on the compiler for parsing ASTs would have to be updated. Although the compiler is not technically considered public API, such breakages are still problematic to push through.
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
I agree that this would be difficult to push through. IMO the benefit of this isn't worth the churn -- the expression grammar is fairly small, so exposing a common interface for identifier-like ASTs is probably enough. |
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
Outdated
Show resolved
Hide resolved
@JoostK there are still some minor things that could be improved, for example wrapping method calls for diagnostics and annotating that with the span including any receivers (like is done for property reads). I think this can be done in another PR though, since this one is very large as is. |
ASTs for property read and method calls contain information about the entire span of the expression, including its receiver. Use cases like a language service and compile error messages may be more interested in the span of the direct identifier for which the expression is constructed (i.e. an accessed property). To support this, this commit adds a `nameSpan` property on - `PropertyRead`s - `SafePropertyRead`s - `PropertyWrite`s - `MethodCall`s - `SafeMethodCall`s The `nameSpan` property already existed for `BindingPipe`s. This commit also updates usages of these expressions' `sourceSpan`s in Ngtsc and the langauge service to use `nameSpan`s where appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for compiler changes.
Caretaker: could you run g3sync? (I think there may be some failures with Codelyzer) |
…36826) ASTs for property read and method calls contain information about the entire span of the expression, including its receiver. Use cases like a language service and compile error messages may be more interested in the span of the direct identifier for which the expression is constructed (i.e. an accessed property). To support this, this commit adds a `nameSpan` property on - `PropertyRead`s - `SafePropertyRead`s - `PropertyWrite`s - `MethodCall`s - `SafeMethodCall`s The `nameSpan` property already existed for `BindingPipe`s. This commit also updates usages of these expressions' `sourceSpan`s in Ngtsc and the langauge service to use `nameSpan`s where appropriate. PR Close #36826
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…ngular#36826) ASTs for property read and method calls contain information about the entire span of the expression, including its receiver. Use cases like a language service and compile error messages may be more interested in the span of the direct identifier for which the expression is constructed (i.e. an accessed property). To support this, this commit adds a `nameSpan` property on - `PropertyRead`s - `SafePropertyRead`s - `PropertyWrite`s - `MethodCall`s - `SafeMethodCall`s The `nameSpan` property already existed for `BindingPipe`s. This commit also updates usages of these expressions' `sourceSpan`s in Ngtsc and the langauge service to use `nameSpan`s where appropriate. PR Close angular#36826
ASTs for property read and method calls contain information about
the entire span of the expression, including its receiver. Use cases
like a language service and compile error messages may be more
interested in the span of the direct identifier for which the
expression is constructed (i.e. an accessed property). To support this,
this commit adds a
nameSpan
property onPropertyRead
sSafePropertyRead
sPropertyWrite
sMethodCall
sSafeMethodCall
sThe
nameSpan
property already existed forBindingPipe
s.This commit also updates usages of these expressions'
sourceSpan
s inNgtsc and the langauge service to use
nameSpan
s where appropriate.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?