Skip to content
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

Ngtsc/ttc/strict #33365

Closed
wants to merge 3 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Oct 23, 2019

Takeover of #33273 for faster round trips.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 23, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Oct 23, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/strict branch 3 times, most recently from 140e771 to 4914800 Oct 24, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/strict branch from 4914800 to 899c6ee Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 24, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Oct 24, 2019

@alxhub alxhub marked this pull request as ready for review Oct 24, 2019
@alxhub alxhub requested a review from angular/fw-compiler as a code owner Oct 24, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/strict branch from 899c6ee to 24fd54c Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 24, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@kara
kara approved these changes Oct 24, 2019
Copy link
Contributor

kara left a comment

LGTM, some minor nits in comments

packages/compiler-cli/src/ngtsc/typecheck/src/api.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/transformers/api.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/transformers/api.ts Outdated Show resolved Hide resolved
@googlebot googlebot added cla: no and removed cla: yes labels Oct 24, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Oct 24, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    failure status "ci/circleci: test" is failing
    failure status "ci/circleci: test_ivy_aot" is failing
    pending forbidden labels detected: PR action: cleanup
    pending status "ci/circleci: integration_test" is pending
    pending status "ci/circleci: test_aio_local" is pending
    pending status "ci/circleci: test_aio_local_viewengine" is pending
    pending status "ci/circleci: test_docs_examples" is pending
    pending status "ci/circleci: test_docs_examples_ivy" is pending
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

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.

JoostK and others added 3 commits Oct 19, 2019
For elements that have a text attribute, it may happen that the element
is matched by a directive that consumes the attribute as an input. In
that case, the template type checker will validate the correctness of
the attribute with respect to the directive's declared type of the
input, which would typically be `boolean` for the `disabled` input.
Since empty attributes are assigned the empty string at runtime, the
template type checker would report an error for this template.

This commit introduces a strictness flag to help alleviate this
particular situation, effectively ignoring text attributes that happen
to be consumed by a directive.
View Engine correctly infers the type of local refs to directives or to
<ng-template>s, just not to DOM nodes. This commit splits the
checkTypeOfReferences flag into two separate halves, allowing the compiler
to align with this behavior.
The template type checking abilities of the Ivy compiler are far more
advanced than the level of template type checking that was previously
done for Angular templates. Up until now, a single compiler option
called "fullTemplateTypeCheck" was available to configure the level
of template type checking. However, now that more advanced type checking
is being done, new errors may surface that were previously not reported,
in which case it may not be feasible to fix all new errors at once.

Having only a single option to disable a large number of template type
checking capabilities does not allow for incrementally addressing newly
reported types of errors. As a solution, this commit introduces some new
compiler options to be able to enable/disable certain kinds of template
type checks on a fine-grained basis.
@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/strict branch from a287925 to ce3457c Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 24, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

AndrewKushnir added a commit that referenced this pull request Oct 24, 2019
…3365)

View Engine correctly infers the type of local refs to directives or to
<ng-template>s, just not to DOM nodes. This commit splits the
checkTypeOfReferences flag into two separate halves, allowing the compiler
to align with this behavior.

PR Close #33365
AndrewKushnir added a commit that referenced this pull request Oct 24, 2019
The template type checking abilities of the Ivy compiler are far more
advanced than the level of template type checking that was previously
done for Angular templates. Up until now, a single compiler option
called "fullTemplateTypeCheck" was available to configure the level
of template type checking. However, now that more advanced type checking
is being done, new errors may surface that were previously not reported,
in which case it may not be feasible to fix all new errors at once.

Having only a single option to disable a large number of template type
checking capabilities does not allow for incrementally addressing newly
reported types of errors. As a solution, this commit introduces some new
compiler options to be able to enable/disable certain kinds of template
type checks on a fine-grained basis.

PR Close #33365
mhevery added a commit to mhevery/angular that referenced this pull request Oct 25, 2019
…3365)

For elements that have a text attribute, it may happen that the element
is matched by a directive that consumes the attribute as an input. In
that case, the template type checker will validate the correctness of
the attribute with respect to the directive's declared type of the
input, which would typically be `boolean` for the `disabled` input.
Since empty attributes are assigned the empty string at runtime, the
template type checker would report an error for this template.

This commit introduces a strictness flag to help alleviate this
particular situation, effectively ignoring text attributes that happen
to be consumed by a directive.

PR Close angular#33365
mhevery added a commit to mhevery/angular that referenced this pull request Oct 25, 2019
…gular#33365)

View Engine correctly infers the type of local refs to directives or to
<ng-template>s, just not to DOM nodes. This commit splits the
checkTypeOfReferences flag into two separate halves, allowing the compiler
to align with this behavior.

PR Close angular#33365
mhevery added a commit to mhevery/angular that referenced this pull request Oct 25, 2019
The template type checking abilities of the Ivy compiler are far more
advanced than the level of template type checking that was previously
done for Angular templates. Up until now, a single compiler option
called "fullTemplateTypeCheck" was available to configure the level
of template type checking. However, now that more advanced type checking
is being done, new errors may surface that were previously not reported,
in which case it may not be feasible to fix all new errors at once.

Having only a single option to disable a large number of template type
checking capabilities does not allow for incrementally addressing newly
reported types of errors. As a solution, this commit introduces some new
compiler options to be able to enable/disable certain kinds of template
type checks on a fine-grained basis.

PR Close angular#33365
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.