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

[CP][stable channel] ec48e8f - DDC: Fix missing nullability from recursive type hierarchies #45907

Closed
nshahan opened this issue May 4, 2021 · 7 comments
Assignees
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@nshahan
Copy link
Contributor

nshahan commented May 4, 2021

commit(s) to merge: ec48e8f

merge instructions: clean merge

What is the issue: DDC uses a different code path when compiling mixins or extends clauses that lead to a circular reference of the class itself. That code path was losing nullability information (nullable, or legacy) of the types when they appear in those positions.

What is the fix: Stop ignoring the nullability and emit them properly in those code patterns.

Why cherrypick: This is a correctness issue, mostly noticeable when running in sound null safety mode. The bug has been present since we unforked the SDK but it was noticed and reported recently by an external Flutter Web user.

Risk: Low risk because the fix targets a code path that is only used by circular class hierarchies and then the hierarchy must include a nullable type for the change to make any difference.

Link to original issue(s): #45767

/cc @kevmoo @mit-mit @whesse @athomas @vsmenon @franklinyow @sigmundch

@nshahan nshahan added merge-to-beta cherry-pick-review Issue that need cherry pick triage to approve labels May 4, 2021
@sigmundch
Copy link
Member

@leafpetersen - we are curious to hear your thoughts on the severity of the issue here.

I'm ambivalent this time around. I agree it's low risk and I'd have gone for it a couple weeks back, but given the higher bar for cherry-picking at this time, I'm inclined to wait for the next release and not cherry-pick.

@leafpetersen
Copy link
Member

I'm still supportive of a cherry pick. The change is DDC only, so the risk is highly ring-fenced. The problem is somewhat esoteric, but it was found organically, and if I understand correctly this bug will be more likely to surface as apps turn on sound null safety.

@mit-mit mit-mit added the area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. label May 5, 2021
@franklinyow
Copy link
Contributor

CC: @kf6gpe

@kf6gpe
Copy link

kf6gpe commented May 6, 2021

Are we discussing this for Dart 2.13 beta? If so, the window opens again om the week of 5/21. Can we wait that long given the fact that it's an esoteric case? Or hotfix in Dart and pick it up in Flutter when we hotfix Flutter that week?

@franklinyow
Copy link
Contributor

Discussed in SCURM meeting. This can wait until 5/12. We are approving this for 5/21 window

I will update the label and title in a later date.

@franklinyow franklinyow self-assigned this May 6, 2021
@franklinyow franklinyow added merge-to-stable cherry-pick-approved Label for approved cherrypick request and removed merge-to-beta labels May 13, 2021
@franklinyow franklinyow changed the title [CP][beta channel] ec48e8f - DDC: Fix missing nullability from recursive type hierarchies [CP][stable channel] ec48e8f - DDC: Fix missing nullability from recursive type hierarchies May 13, 2021
@franklinyow
Copy link
Contributor

Description:
Fix missing nullability from recursive type hierarchies, see #45767 for detail

@franklinyow franklinyow assigned athomas and unassigned franklinyow May 18, 2021
@athomas
Copy link
Member

athomas commented May 21, 2021

Merged to stable 375a2d7 (2.13.1).

@athomas athomas closed this as completed May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Use area-infrastructure for SDK infrastructure issues, like continuous integration bot changes. cherry-pick-approved Label for approved cherrypick request cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Development

No branches or pull requests

7 participants