Skip to content

fix transform assert#31568

Merged
pq merged 3 commits intoflutter:masterfrom
pq:fix_matrix_assert
Apr 25, 2019
Merged

fix transform assert#31568
pq merged 3 commits intoflutter:masterfrom
pq:fix_matrix_assert

Conversation

@pq
Copy link
Contributor

@pq pq commented Apr 24, 2019 •
edited
Loading

Description

Fixes an assert that was bogus.

Flagged by the newly updated unrelated_type_equality_checks lint in this failing try:

https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8915275287386529616/+/steps/analyze_flutter/0/stdout

Cheers @srawlins for the fix!

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@pq pq mentioned this pull request Apr 24, 2019
Closed
9 tasks
@pq
Copy link
Contributor Author

pq commented Apr 24, 2019

@dnfield, @jonahwilliams: any thoughts on the failures here?

@pq pq requested a review from jonahwilliams April 24, 2019 22:52
@jonahwilliams
Copy link
Contributor

jonahwilliams commented Apr 24, 2019

Part of the reason this ends up being breaking is that we have inadvertently created scenarios where our matrices are non-invertible. It's also not clear whether turning this on will effect users as well, as it might end up being a large breaking change.

We would need to:

  • Figure out what widgets/behaviors are causing these matrices to be created and why they don't break elsewhere. Perhaps they go unused or is test only behavior?
  • From there, figure out what the likely impact on users is for re-enabling this assert. We could try running google3 with the required framework changes and see if it affects applications.

I don't think it will be obvious to most users what the problem is if this assert starts firing in their application, so we should be careful.

cc @Hixie @goderbauer

@jonahwilliams jonahwilliams added the framework flutter/packages/flutter repository. See also f: labels. label Apr 24, 2019
@pq
Copy link
Contributor Author

pq commented Apr 25, 2019

@Hixie, @goderbauer: assuming this is not an easy call to make and in the interest of not blocking an analyzer roll could we consider adding a comment and:

  1. removing the assert, or
  2. adding an ignore

?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Apr 25, 2019

Since the assert is functionally diasbled we can comment it out here to unblock analyzer

@pq
Copy link
Contributor Author

pq commented Apr 25, 2019

Awesome. Thanks @jonahwilliams !

jonahwilliams
jonahwilliams approved these changes Apr 25, 2019
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pq pq merged commit 5ec3365 into flutter:master Apr 25, 2019
@pq pq deleted the fix_matrix_assert branch April 25, 2019 17:56
@Hixie
Copy link
Contributor

Hixie commented May 3, 2019

Was a bug filed to fix this? We should fix this soon; adding commented-out code is strictly against our style guide.

@jonahwilliams jonahwilliams mentioned this pull request Jan 3, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pq @jonahwilliams @Hixie @googlebot