Turned "unnecessary value" and "move of const" lints to errors on mac #36910

gaaclarke · 2022-10-20T23:44:14Z

Fixed up some new violations to the "move const values" lint and the "unnecessary value" lint and made it an error on CI for host_debug on mac.

If this fails in Post submit checks let me know, it's possible a violation was introduced between authoring and landing the check

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2022-10-21T05:02:44Z

@zanderso @jmagman

Re: #36885

Here we can turn this on selectively as an error and I think we should keep the check in .clang-tidy so that clang-tidy aware tools still work (analyzers and clang tidy fix). I'm not going to sit and watch this finish but works locally, I'll check on it tomorrow. Here's the gist anyway.

zanderso · 2022-10-21T18:22:47Z

tools/clang_tidy/lib/src/options.dart

+/// Adds warnings as errors for only specific runs. This is helpful if migrating one platform at a time.
+String? _platformSpecificWarningsAsErrors(String targetVariant) {
+ if (targetVariant == 'host_debug' && io.Platform.isMacOS) {
+ return 'performance-move-const-arg,performance-unnecessary-value-param';


If we do in fact need this, I'd prefer hoisting this logic out into the lint.sh script to make it more visible.

The reason I didn't do that is because it's really ugly doing the same logic in sh. The thought being that having dart code is easier to get right and maintain instead of sh.

I could make another flag --mac-host-warnings-as-errors=foobar.

With this PR, these lints are added, like, three layers deep (.clang-tidy -> lint.sh -> options.dart), so I'm more concerned that people are going to get confused about how/why they're enabled than I am about adding logic to the shell script. --mac-host-warnings-as-errors is kind of oddly specific, but at least makes the additional lints easier to find.

I agree this is really buried. I think I should merge #36885 and then either

Add these two lints back to the checks when all the platforms issues are resolved, as was done with the other linters.
or

Create override .clang-tidy files in the different embedder directories so we can add linter checks to different platforms without needing to fix them everywhere. I believe clang-tidy will prefer the .clang-tidy file closest to the file being linted.

--mac-host-warnings-as-errors is kind of oddly specific, but at least makes the additional lints easier to find.

Yea it's weird. Once we get a solid foothold it should be easier to land the other platforms in another PR while this one holds the line for the most part. I'll make that change.

I think I should merge #36885 and then either

I just don't understand how that is helping anyone. The thesis for removing it was that it was confusing and it wasn't benefiting anyone. I've shown that it is doing what I expected and it is beneficial in limited cases (albeit not as many as I had hoped). I agree we need to make it better by turning it into an error on ci.

Add these two lints back to the checks when all the platforms issues are resolved, as was done with the other linters.

That needlessly makes the task hard for no reason. In order to do it all at once I need to run clang-tidy-fix 5 times on 3 different platforms, hand fix the automated fix since it isn't 100% perfect, get it all reviewed and landed all within the span of no one introducing another violation. Otherwise have to start the process all over again.

Having multiple .clang-tidy files also has it's own problems where managing multiple files is a pain and the hope is that this is a special case and a transitory one.

Done, moved the warnings-as-errors declaration to lint.sh. Leaving the thread unresolved since there was some other comments.

I just don't understand how that is helping anyone.

It helps because the expected behavior that adding a check to the .clang-tidy file will surface regressions to that lint... somewhere. Without needing to set up an IDE with clang-tidy, if anyone does this, and without needing to know to run --fix. If we don't merge #36885 then also need to move modernize-use-default-member-init to WarningsAsErrors as that's a pretty clear example of the confusion. Generally when we add new lints, errors or warnings, in other linter systems, it also involves fixing the existing violations of that lint. I know these two lints aren't totally cleaned up though on all platforms, and that's the issue here.

However we obviously disagree here, so I hope people notice the behavior of --mac-host-warnings-as-errors and any similar platform-specific lints going forward.

zanderso

lgtm w/ issue to track cleanup.

zanderso · 2022-10-21T19:57:45Z

ci/lint.sh

@@ -32,6 +32,7 @@ SRC_DIR="$(cd "$SCRIPT_DIR/../.."; pwd -P)"
 FLUTTER_DIR="$(cd "$SCRIPT_DIR/.."; pwd -P)"
 DART_BIN="${SRC_DIR}/third_party/dart/tools/sdks/dart-sdk/bin"
 DART="${DART_BIN}/dart"
+MAC_HOST_WARNINGS_AS_ERRORS="performance-move-const-arg,performance-unnecessary-value-param"


Please add a TODO and file an issue to clean this up when done.

flutter/flutter#113848

zanderso · 2022-10-21T20:30:32Z

@gaaclarke These migrations are difficult, so it makes sense to me that we're having this discussion. Another way to slice up the work, rather than platform-by-platform, is to use the NOLINTBEGIN/END annotations (there might also be a single annotation that applies to a whole file) to instead work file-by-file. After you're done with enabling the lints that you're currently working on. I'd like us to land #36885, and use the file-by-file approach next time.

gaaclarke · 2022-10-21T21:16:07Z

@zanderso that makes sense. What we've done in the past with the formatter is print out the patch that will fix the issue so you don't have to recalculate it on your own machine. I think that's what we should do since here too since it's unreasonable to make people run N instances of clang tidy on 3 different machines to duplicate work that's being done on a bot already.

jmagman · 2022-10-21T21:20:56Z

I'd like us to land #36885, and use the file-by-file approach next time.

That will not work in conjunction with this PR, as @gaaclarke pointed out in #36885 (review). Either all warnings are errors, or we prefer this pattern where only checks in WarningsAsErrors + whatever exceptions like this are added to lint.sh.

jmagman · 2022-10-21T21:23:00Z

After you're done with enabling the lints that you're currently working on. I'd like us to land #36885, and use the file-by-file approach next time.

Or you meant land that PR after these two lint can be turned on for all platforms?

zanderso · 2022-10-21T21:30:07Z

Or you meant land that PR after these two lint can be turned on for all platforms?

Yes.

…ors on mac (flutter/engine#36910)

gaaclarke force-pushed the unnecessary-value-error branch 2 times, most recently from 510654c to 814d8f9 Compare October 21, 2022 04:55

flutter-dashboard bot added the embedder Related to the embedder API label Oct 21, 2022

gaaclarke changed the title ~~Turned unnecessary value and move of const to errors.~~ Turned "unnecessary value" and "move of const" lints to errors on mac Oct 21, 2022

gaaclarke mentioned this pull request Oct 21, 2022

Only run clang-tidy warning checks reported as errors #36885

Merged

8 tasks

gaaclarke force-pushed the unnecessary-value-error branch from 814d8f9 to eab8459 Compare October 21, 2022 16:08

Turned unnecessary value and move of const to errors for mac host_debug.

aad8d17

gaaclarke force-pushed the unnecessary-value-error branch from eab8459 to aad8d17 Compare October 21, 2022 17:03

gaaclarke marked this pull request as ready for review October 21, 2022 17:50

gaaclarke requested review from jmagman and zanderso October 21, 2022 17:50

zanderso reviewed Oct 21, 2022

View reviewed changes

surfaced the warnings as errors into the lint.sh file

c236920

gaaclarke requested a review from zanderso October 21, 2022 19:44

zanderso approved these changes Oct 21, 2022

View reviewed changes

gaaclarke mentioned this pull request Oct 21, 2022

Make "unnecessary value" and "move of const" linter checks errors on all platforms flutter/flutter#113848

Closed

added TODO

d2ca757

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2022

added extra comment

fce2175

auto-submit bot merged commit f78a891 into flutter:main Oct 22, 2022

engine-flutter-autoroll mentioned this pull request Oct 22, 2022

Roll Flutter Engine from e35f850102e6 to 30bb44616bac (3 revisions) flutter/flutter#113875

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2022

f78a891d0 Turned "unnecessary value" and "move of const" lints to err…

eacb4de

…ors on mac (flutter/engine#36910)

jmagman mentioned this pull request Nov 17, 2022

Only run clang-tidy warning checks reported as errors #37698

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turned "unnecessary value" and "move of const" lints to errors on mac #36910

Turned "unnecessary value" and "move of const" lints to errors on mac #36910

gaaclarke commented Oct 20, 2022 •

edited

Loading

gaaclarke commented Oct 21, 2022

zanderso Oct 21, 2022

gaaclarke Oct 21, 2022

gaaclarke Oct 21, 2022

zanderso Oct 21, 2022 •

edited

Loading

jmagman Oct 21, 2022

gaaclarke Oct 21, 2022

gaaclarke Oct 21, 2022

jmagman Oct 21, 2022

zanderso left a comment

zanderso Oct 21, 2022

gaaclarke Oct 21, 2022

gaaclarke Oct 21, 2022

zanderso commented Oct 21, 2022

gaaclarke commented Oct 21, 2022

jmagman commented Oct 21, 2022

jmagman commented Oct 21, 2022

zanderso commented Oct 21, 2022

Turned "unnecessary value" and "move of const" lints to errors on mac #36910

Turned "unnecessary value" and "move of const" lints to errors on mac #36910

Conversation

gaaclarke commented Oct 20, 2022 • edited Loading

Pre-launch Checklist

gaaclarke commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso Oct 21, 2022 • edited Loading

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

zanderso left a 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

zanderso commented Oct 21, 2022

gaaclarke commented Oct 21, 2022

jmagman commented Oct 21, 2022

jmagman commented Oct 21, 2022

zanderso commented Oct 21, 2022

gaaclarke commented Oct 20, 2022 •

edited

Loading

zanderso Oct 21, 2022 •

edited

Loading