format: add checks for type alias #11634

tomocy · 2020-06-18T09:51:01Z

Commit Message: format: add checks for type alias
Additional Description: N/A
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Fixes #9840

htuch · 2020-06-18T12:55:07Z

tools/code_format/check_format.py

@@ -696,6 +696,15 @@ def checkSourceLine(line, file_path, reportError):
        reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " +
                    "Grpc::GoogleGrpcContext. See #8282")

+  if not line.startswith("using"):
+    smart_ptr_m = re.search("std::(unique_ptr|shared_ptr)<(.*?)>", line)


Is this a bit too strict? I understand now wanting to have verbose unique_ptr for commonly used types, but the occasional use of unique_ptr, in particular for non-Envoy types, makes sense.

I’m going to add an allowed type list (eg. [int, std::string]) and report an error for the types which are not included in that list. I suppose it handles normal use cases well.

jmarantz

It would be nice to actually fix these errors if possible. Note that gazillion format-checks being reported in CI on this PR; I bet you don't want to fix them by hand :)

The problem is that some of the violations found may not actually have aliases in scope of their use, and it would probably be beyond the scope of the format-fixer to verify that. Usually that won't be a problem if we always declare FooPtr wherever the class definition for Foo is. But sometimes (not often) we need to pre-declare Foo, so it's not perfect.

And currently, in my experience the format fixer never breaks code, so I run it before committing without bothering to re-compile/re-test locally (CI is of course still run). I don't want to break that pattern.

So my suggestion is to have an "--aggressive" switch on the format fixer, or similar, which does risky changes, including swapping in the assumed type alias. Then you can fix the large number of errors in this PR's CI: https://dev.azure.com/cncf/envoy/_build/results?buildId=42610&view=logs&j=9fd74226-df5b-5f29-39da-76fc341cc763&t=068b98b1-acf0-52d0-fc6e-f6d1a5b1d5ef&l=4223

jmarantz · 2020-06-19T11:55:04Z

tools/code_format/check_format.py

@@ -352,6 +398,9 @@ def whitelistedForUnpackTo(file_path):
      "./source/common/protobuf/utility.cc", "./source/common/protobuf/utility.h"
  ]

+def whitelistedForNonTypeAlias(name):
+  return re.match(r".*\[\]$", name) or re.match("^std::vector<.*", name) or re.match(f"^({'|'.join(NON_TYPE_ALIAS_ALLOWED_TYPES)})$", name)


suggest precompiling this regex rather than re-compiling the regex for every name checked.

Also why not combine the first two cases with the third in the precompiled regex so we just have to do one pre-compiled regex check?

jmarantz · 2020-06-19T11:57:13Z

tools/testdata/check_format/using_type_alias.cc

+  using ConnectionOptRef = absl::optional<std::reference_wrapper<Connection>>;
+};
+} // namespace Network
+} // namespace Envoy


add trailing newline

Signed-off-by: tomocy <[email protected]>

tomocy · 2020-06-22T08:02:21Z

Thank you for your review. I considered adding --aggressive flag but this flag would not be enabled as long as those types reported in CI are aliased. I'm looking for a tool or a way that automatically add type alias and is invoked by check_format.py fix. Thinking that the code format fixer never breaks code, It is also good to run the tool once before this change is merged.

jmarantz · 2020-06-22T12:08:22Z

I'm having trouble parsing your phrase:

but this flag would not be enabled as long as those types reported in CI are aliased

That makes me want to clarify a few things.

This PR cannot be merged until all the format errors are resolved. You'll want automation to do that. There are lots of examples in check_format.py where fixes are applied automatically.
The --aggressive flag would be set by a human when running check_format fix --aggressive, and the idea here would be that the PR might be left in an unbuildable state, which you would then manually fix and test before committing.

Does that makes sense?

jmarantz

/wait

jmarantz · 2020-06-22T12:15:37Z

tools/code_format/check_format.py

@@ -696,6 +747,17 @@ def checkSourceLine(line, file_path, reportError):
        reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " +
                    "Grpc::GoogleGrpcContext. See #8282")

+  if not re.search("using .* = .*;", line):


precompile all the regexes.

jmarantz · 2020-06-22T12:16:08Z

tools/code_format/check_format.py

@@ -696,6 +747,17 @@ def checkSourceLine(line, file_path, reportError):
        reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " +
                    "Grpc::GoogleGrpcContext. See #8282")

+  if not re.search("using .* = .*;", line):
+    smart_ptr_m = re.finditer("std::(unique_ptr|shared_ptr)<(.*?)>", line)


what does the _m mean? Spell that out?

Signed-off-by: tomocy <[email protected]>

tomocy · 2020-06-23T06:09:07Z

I mean 1. I think that it needs C++ AST to resolve those errors perfectly and is difficult to do only with Python scripts. I am looking for a way to use the AST and fix it, and if it is not possible, I will replace those lines with scripts and declare type aliases manually.

Signed-off-by: tomocy <[email protected]>

tomocy · 2020-06-25T11:31:56Z

I have changed my mind (too many to fix :)) and let it check the format in the added line as pre-push.

jmarantz

I have to admit I'm not crazy about this approach, because when you add something like this it adds unexpected burden to anyone modifying a file with otherwise unchecked errors. If we can, I'd prefer to just fix everything and keep it fixed.

IMO, as I suggested above, I think you can do a 95% job at fixing everything automatically in Python without the C++ AST. You should just flag-protect it so that by default, the fix-format mode does not make risky changes.

But for this PR you can go ahead and let it make risky easy changes and then just fix whatever breaks when you run tests.

jmarantz · 2020-06-27T19:29:35Z

support/hooks/pre-push

@@ -14,7 +14,7 @@ DUMMY_SHA=0000000000000000000000000000000000000000
 echo "Running pre-push check; to skip this step use 'push --no-verify'"

 while read LOCAL_REF LOCAL_SHA REMOTE_REF REMOTE_SHA
-do
+do    


remove trailing whitespace

jmarantz · 2020-06-27T19:30:09Z

tools/code_format/check_format.py

@@ -168,6 +168,7 @@
  "extensions/filters/network/common",
  "extensions/filters/network/common/redis",
 }
+


jmarantz · 2020-06-27T20:36:31Z

/wait

Signed-off-by: tomocy <[email protected]>

…checks-for-type-alias Signed-off-by: tomocy <[email protected]>

Signed-off-by: tomocy <[email protected]>

tomocy · 2020-07-14T09:45:52Z

@jmarantz ping. It is getting ready to be reviewed.

jmarantz

quick drive-by: is it possible that you have added using nicknames that are not used?

This is a tough PR to review of course because it's so massive, and will be a difficult slog staying ahead of merge conflicts.

One suggestion:

break this up into (say) 50-file fragments without the tools. These should be easy to knock off a chunk at a time.
review the tools separately, where we'll want to talk in detail about python and whitelists and error messages and how to test, etc

You can phase in all the .cc/.h changes while we iterate on the python. The final python checkin should only need cc/h changes for violations that were added after you started this process.

jmarantz · 2020-07-14T14:38:44Z

source/common/buffer/buffer_impl.h

@@ -484,6 +489,8 @@ class BufferFragmentImpl : NonCopyable, public BufferFragment {
  const std::function<void(const void*, size_t, const BufferFragmentImpl*)> releasor_;
 };

+using BufferFragmentImplPtr = std::unique_ptr<BufferFragmentImpl>;


it's not obvious to me why this was needed. It does not appear to be used.

jmarantz · 2020-07-14T14:39:02Z

source/common/buffer/buffer_impl.h

@@ -597,6 +604,8 @@ class OwnedImpl : public LibEventInstance {
  OverflowDetectingUInt64 length_;
 };

+using OwnedImplPtr = std::unique_ptr<OwnedImpl>;


is this used?

tomocy · 2020-07-15T12:00:14Z

I added type aliases every the compiler failed to build due to the undefined error. But I did it manually so a part of those may not be used.

My understanding is that I will checkout two kinds of branches where I cherry-pick the changes about type aliasing by 50 file fragments while I push the changes about Python tools.

Is it correct?

jmarantz · 2020-07-15T12:06:44Z

RE "unused type declarations": I thought we had talked about an automated approach to resolving these (even that automation was not robust enough to never fail). Sounds like you did a lot of manual work. Sorry about that. If we wind up with extra type aliases I won't get too worried.

RE "branches and cherry-picks": I am really barely functional at git and do things in kind of a low-tech way. But what I would do is something like:

merge master
git diff >/tmp/all_changes.diff
edit all_changes.diff in a text editor, splitting it into K digestible chunks, especially the one with the tooling chnages
open up K new branches from master, and patch each of the diff files, e.g. with "patch -p1 </tmp/changes_23.diff"
make sure each of the branches pass tests on their own with "bazel test //test/...".
start streaming out reviews.

Even if you wanted to make K be fairly small that would be fine; the one I want to review first is the one with the tooling changes, even though we won't be able to merge it until all the others are done.

Does that make sense?

tomocy · 2020-07-15T13:02:30Z

Thank you, it makes sense. I am trying in the way.

Commit Message: format: use type alias Additional Description: N/A Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Part of #11634 Signed-off-by: tomocy <[email protected]>

Commit Message: format: use type alias Additional Description: N/A Risk Level: N/A Testing: N/A Docs Changes: N/A Release Notes: N/A Part of #11634 Signed-off-by: tomocy <[email protected]>

stale · 2020-07-25T18:19:14Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Commit Message: format: use type alias Additional Description: N/A Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Part of envoyproxy#11634 Signed-off-by: tomocy <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>

Commit Message: format: use type alias Additional Description: N/A Risk Level: N/A Testing: N/A Docs Changes: N/A Release Notes: N/A Part of envoyproxy#11634 Signed-off-by: tomocy <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>

stale · 2020-08-01T18:58:11Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Commit Message: format: use type alias Additional Description: N/A Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Part of envoyproxy#11634 Signed-off-by: tomocy <[email protected]> Signed-off-by: scheler <[email protected]>

Commit Message: format: use type alias Additional Description: N/A Risk Level: N/A Testing: N/A Docs Changes: N/A Release Notes: N/A Part of envoyproxy#11634 Signed-off-by: tomocy <[email protected]> Signed-off-by: scheler <[email protected]>

htuch reviewed Jun 18, 2020

View reviewed changes

htuch assigned jmarantz Jun 18, 2020

htuch requested a review from jmarantz June 18, 2020 12:55

tomocy force-pushed the format-add-checks-for-type-alias branch from e8eac94 to 0bd3b19 Compare June 19, 2020 10:40

jmarantz reviewed Jun 19, 2020

View reviewed changes

tomocy added 6 commits June 22, 2020 07:49

format: add checks for type alias

7ddce42

Signed-off-by: tomocy <[email protected]>

modify using type alias match

9159b57

Signed-off-by: tomocy <[email protected]>

modify to find all of non type alias type in line

aa2dd3c

Signed-off-by: tomocy <[email protected]>

add non type alias allowed type list

c70a52e

Signed-off-by: tomocy <[email protected]>

add trailing newline

bf581d4

Signed-off-by: tomocy <[email protected]>

compile and combine regexs

658dc57

Signed-off-by: tomocy <[email protected]>

tomocy force-pushed the format-add-checks-for-type-alias branch from 0bd3b19 to 658dc57 Compare June 22, 2020 07:50

jmarantz reviewed Jun 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 22, 2020

tomocy added 2 commits June 23, 2020 05:06

compile regexs

a49224b

Signed-off-by: tomocy <[email protected]>

refactor names

0387590

Signed-off-by: tomocy <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 23, 2020

tomocy added 6 commits June 23, 2020 17:14

modify regex

06fe468

Signed-off-by: tomocy <[email protected]>

add check line format hook

143d0db

Signed-off-by: tomocy <[email protected]>

add line format check for type alias

cd5235b

Signed-off-by: tomocy <[email protected]>

modify to check only source lines

09a8d13

Signed-off-by: tomocy <[email protected]>

fix

7aef23a

Signed-off-by: tomocy <[email protected]>

format

04c7681

Signed-off-by: tomocy <[email protected]>

jmarantz reviewed Jun 27, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 27, 2020

tomocy added 3 commits July 13, 2020 14:36

fix

47630b3

Signed-off-by: tomocy <[email protected]>

Merge branch 'master' of github.com:envoyproxy/envoy into format-add-…

1e9fffd

…checks-for-type-alias Signed-off-by: tomocy <[email protected]>

fix

5c20a6f

Signed-off-by: tomocy <[email protected]>

tomocy marked this pull request as ready for review July 14, 2020 09:45

jmarantz reviewed Jul 14, 2020

View reviewed changes

This was referenced Jul 15, 2020

format: add checks for type aliases #12099

Closed

format: use type alias #12104

Merged

format: use type alias #12125

Merged

format: use type alias #12126

Merged

junr03 pushed a commit that referenced this pull request Jul 16, 2020

use type alias (#12126)

acb8847

Commit Message: format: use type alias Additional Description: N/A Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Part of #11634 Signed-off-by: tomocy <[email protected]>

tomocy mentioned this pull request Jul 17, 2020

format: use type alias #12146

Merged

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 25, 2020

stale bot closed this Aug 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

format: add checks for type alias #11634

format: add checks for type alias #11634

tomocy commented Jun 18, 2020

htuch Jun 18, 2020

tomocy Jun 19, 2020

jmarantz left a comment

jmarantz Jun 19, 2020

tomocy Jun 22, 2020

jmarantz Jun 19, 2020

tomocy Jun 22, 2020

tomocy commented Jun 22, 2020 •

edited

Loading

jmarantz commented Jun 22, 2020

jmarantz left a comment

jmarantz Jun 22, 2020

tomocy Jun 23, 2020

jmarantz Jun 22, 2020

tomocy Jun 23, 2020

tomocy commented Jun 23, 2020

tomocy commented Jun 25, 2020 •

edited

Loading

jmarantz left a comment

jmarantz Jun 27, 2020

jmarantz Jun 27, 2020

jmarantz commented Jun 27, 2020

tomocy commented Jul 14, 2020

jmarantz left a comment

jmarantz Jul 14, 2020

jmarantz Jul 14, 2020

tomocy commented Jul 15, 2020

jmarantz commented Jul 15, 2020

tomocy commented Jul 15, 2020

stale bot commented Jul 25, 2020

stale bot commented Aug 1, 2020

format: add checks for type alias #11634

format: add checks for type alias #11634

Conversation

tomocy commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz 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

Choose a reason for hiding this comment

tomocy commented Jun 22, 2020 • edited Loading

jmarantz commented Jun 22, 2020

jmarantz 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

Choose a reason for hiding this comment

tomocy commented Jun 23, 2020

tomocy commented Jun 25, 2020 • edited Loading

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Jun 27, 2020

tomocy commented Jul 14, 2020

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomocy commented Jul 15, 2020

jmarantz commented Jul 15, 2020

tomocy commented Jul 15, 2020

stale bot commented Jul 25, 2020

stale bot commented Aug 1, 2020

tomocy commented Jun 22, 2020 •

edited

Loading

tomocy commented Jun 25, 2020 •

edited

Loading