Page MenuHomePhabricator

[clang-tidy] modernize-use-trailing-return-type check
ClosedPublic

Authored by bernhardmgruber on Dec 30 2018, 9:44 AM.

Details

Summary

The new clang-tidy pass modernize-use-trailing-return rewrites function signatures to use a trailing return type.

A fair amount of tests are included.

Does not work on return types which span locations before and after the function name (e.g. functions returning function pointers). The pass may fail if the return types are from missing headers (e.g. when clang-tidy is run without a compilation database or all needed include directories)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bernhardmgruber marked 2 inline comments as done.
  • rebased onto current master
  • implemented a basic check for name collisions of unqualified names in the return type with function arugment names using RecursiveASTVisitor
  • moved retrieval of FunctionTypeLoc out of findTrailingReturnTypeSourceLocation()
  • replaced F.getReturnType().hasLocalQualifiers() by custom function hasAnyNestedLocalQualifiers(), as the former does not work if the qualifiers are not on the topmost type. E.g.: const int*. This is a PointerType without qualifiers, the const qualifier is part of the nested pointee type.
  • inhibiting the rewrite, if the topmost return type is a decltype expression. the source range in this case does not include the expression in parenthesis after the decltype
  • inserting an additional space after auto in case there was no space between the return type and the function name. E.g.: int*f();
  • extended documentation with known limitations
  • added more tests

I just noticed, i don't like the naming choice in the check.
UseTrailingReturn what is the "trailing return"?
This is about "trailing return *type*", no?

bernhardmgruber marked 2 inline comments as done.Feb 27 2019, 7:00 AM

Hi guys!

It took me far too long to come up with an update. Honestly, I was quite demotivated as it turned out preventing name collisions of unqualifed names after the rewrite was more difficult than I thought. Especially because this error occurs sparsely when I test the check on bigger code bases. The current approach with the AST visitor seems to work best and maybe I can even extend it at some point to automatically qualify colliding names. But for now, I would be really glad if we could achieve a state of the check that you guys can accept and commit.

I marked two lines with comments where I still need some feedback. Apart from that, I will run the check again on LLVM and other larger code bases to see if there are any further issues.

Thank you for your help!

clang-tidy/modernize/UseTrailingReturnCheck.cpp
79

TODO: is this an issue in RecursiveASTVisitor or is this behavior intended? Everywhere else, it calls getDerived().XXX();

336

FIXME: this feels wrong when I wrote it, but it works. I tried to fiddle with the ReturnTypeCVRange to include the next charakter, but I could not get it working without writing tooling::fixit::getText myself. Any ideas?

It took me far too long to come up with an update. Honestly, I was quite demotivated as it turned out preventing name collisions of unqualifed names after the rewrite was more difficult than I thought. Especially because this error occurs sparsely when I test the check on bigger code bases. The current approach with the AST visitor seems to work best and maybe I can even extend it at some point to automatically qualify colliding names. But for now, I would be really glad if we could achieve a state of the check that you guys can accept and commit.

I totally know that feeling! If there is a wrong transformation it would always be catched on compilation, right? I am fine with a check that handles 99% of code correct and we do have other checks that have similar limitations. If you find a way to exclude the wrong cases from transformation even better.

clang-tidy/modernize/UseTrailingReturnCheck.cpp
79

I don't know, but its not a big issue, is it?

336

That only works, because StringRef points within the orignal code buffer. IMHO just adding the space always and letting clang-format do its job is better then this potential out-of-bounds read.
Maybe you could increase the ReturnTypeCVRange by one at the end, then its fine.

bernhardmgruber marked 4 inline comments as done.
bernhardmgruber retitled this revision from [clang-tidy] modernize-use-trailing-return check to [clang-tidy] modernize-use-trailing-return-type check.
  • renamed the check to modernize-use-trailing-return-type
  • fixed the out-of-bounds access to check whether there is a space after the return type
clang-tidy/modernize/UseTrailingReturnCheck.cpp
336

I now retrieve the space after the return type explicitely. Using clang-format afterwards is not always possible on the code bases I work with, so I would like to not rely on it.

  • renamed the check to modernize-use-trailing-return-type

Thanks!

From my side only the nits are left.

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
94 ↗(On Diff #188621)

Nit: const is only used for references or pointers.

154 ↗(On Diff #188621)

Nit: I think this and the next empty line could be ellided. They do not add a lot of value and we try to keep the code dense.

241 ↗(On Diff #188621)

integer bug for big Tokens.size(). Please add an assert(I <= size_t(std::numeric_limits<int>::max()) && "Integer overflow detected") or the like.
Optimally gsl::narrow<>() could be used, which we do not have in LLVM.

bernhardmgruber marked 3 inline comments as done.

Fixed some little nits, thanks @JonasToth!

aaron.ballman added inline comments.Mar 6 2019, 9:28 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
34 ↗(On Diff #189222)

Missing full stop at the end of the sentence.

150 ↗(On Diff #189222)

This list misses an edge case for __restrict. e.g., https://godbolt.org/z/d4WDcA

180 ↗(On Diff #189222)

Missing full stop.

229 ↗(On Diff #189222)

This also misses the __restrict case.

274 ↗(On Diff #189222)

Should this also bail out if the function is main()?

test/clang-tidy/modernize-use-trailing-return-type.cpp
95–97 ↗(On Diff #189222)

This is a rather unexpected transformation, to me.

bernhardmgruber marked 8 inline comments as done.
  • added support for __restrict
  • added two dots at end of comments
aaron.ballman added inline comments.Mar 11 2019, 6:11 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
180–184 ↗(On Diff #189993)

Perhaps I'm a bit dense on a Monday morning, but why should this be a diagnostic? I am worried this will diagnose situations like (untested guesses):

#define const const
const int f(void); // Why no fixit?

#define attr __attribute__((frobble))
attr void g(void); // Why diagnosed as needing a trailing return type?
234 ↗(On Diff #189993)

As a torture test for this, how well does it handle a declaration like:

const long static int volatile unsigned inline long foo();

Does it get the fixit to spit out:

static inline auto foo() -> const unsigned long long int;
293 ↗(On Diff #189993)

This seems incorrect to me; if we cannot get the type source information, why do we assume it has a return type that needs to be turned into a trailing return type?

298 ↗(On Diff #189993)

I think this can turn into an assertion that FTL is valid.

274 ↗(On Diff #189222)

This comment was marked as done, but I don't see any changes or mention of what should happen. I suppose the more general question is: should there be a list of functions that should not have this transformation applied, like program entrypoints? Or do you want to see this check diagnose those functions as well?

test/clang-tidy/modernize-use-trailing-return-type.cpp
95–97 ↗(On Diff #189222)

This comment is marked as done, but there are no changes or explanations.

bernhardmgruber marked 7 inline comments as done and 2 inline comments as done.
  • extracting specifiers from the return type if it consists of a multitoken built-in type, and preprending it to 'auto'.
  • extended matcher to include friend declarations of functions
  • added many tests for return types which contain specifiers
bernhardmgruber marked an inline comment as not done.Mar 18 2019, 7:03 PM

Thank you for the rich feedback @aaron.ballman. I found a solution which seems to work for many of my test cases.

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
180–184 ↗(On Diff #189993)

Because I would also like to rewrite functions which contain macros in the return type. However, I cannot provide a fixit in all cases. Clang can give me a SourceRange without CV qualifiers which seems to work in all my tests so far. But to include CV qualifiers I have to do some manual lexing left and right of the return type SourceRange. If I encounter macros along this way, I bail out because handling these cases becomes compilated very easily.

Your second case does not give a diagnostic, as it is not matched by the check, because it returns void.

234 ↗(On Diff #189993)

I honestly did not believe this compiled until I put it into godbolt. And no, it is not handled.
I added your test as well as a few other ones of this kind. You could also add constexpr or inside classes friend anywhere.

I will try to come up with a solution. I guess the best would be to delete the specifiers from the extracted range type string and readd them in the order of appearance before auto.

298 ↗(On Diff #189993)

I actually found a case where this is null: int unsigned att() __attribute__((cdecl));

268 ↗(On Diff #191233)

I am not sure if this covers all types where a specifier may occur "inside" a type. Can someone come up with something other than a built-in type consisting of at least two tokens?

274 ↗(On Diff #189222)

How strange does

auto main(int argc, const char* argv[]) -> int {
    return 0;
}

look to you?

I think the transformation is valid here. But I can understand that people feel uneasy after typing int main ... for decades. Should we also create an option here to turn it off for main? Or just not transform it? I do not mind. If I want main to start with auto I could also do this transformation manually.

274 ↗(On Diff #189222)

I am sorry for marking it as done. I do not know how people work here exactly and how phabricator behaves. I thought the check boxes are handled for everyone individually. Also, if I add a new comment, it is checked by default.

How are you/most people going to use clang-tidy? Do you run it regularly and automatic? Do you expect it to find zero issues once you applied the check?
In other words, if you do not want to transform some functions, do you need an option to disable the check for these, so it runs clean on the full source code?

Personally, I do not need this behavior, as I run clang-tidy manually once in a while and revert transformations I do not want before checking in the changes.

test/clang-tidy/modernize-use-trailing-return-type.cpp
95–97 ↗(On Diff #189222)

It felt a bit strange initially, but as it is a valid transformation, i have included it. Do you think we should create an option to turn this off?

95–97 ↗(On Diff #189222)

I am sorry.

What do you think about the transformation? Should there be an option to disable transforming extern "C" statements?

aaron.ballman added inline comments.Mar 26 2019, 11:05 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
180–184 ↗(On Diff #189993)

Because I would also like to rewrite functions which contain macros in the return type. However, I cannot provide a fixit in all cases. Clang can give me a SourceRange without CV qualifiers which seems to work in all my tests so far. But to include CV qualifiers I have to do some manual lexing left and right of the return type SourceRange. If I encounter macros along this way, I bail out because handling these cases becomes compilated very easily.

That makes sense, but I am worried about this bailing out because of things that are not CV qualifiers but are typically macros, like attributes. It seems like there should not be a problem providing a fixit in that situation, unless I'm misunderstanding still.

234 ↗(On Diff #189993)

I will try to come up with a solution. I guess the best would be to delete the specifiers from the extracted range type string and readd them in the order of appearance before auto.

Alternatively, if it's easier, you can refuse to add fix-its for the situation and just add a FIXME to handle this should users ever care.

359–364 ↗(On Diff #191233)

This is suspicious and smells like a bug, to me. I can think of no reason why this function would have no type location information just because there's a trailing attribute (I assume you mean RHS here based on the example in your comment above). It may not be your bug to fix, but knowing whether this works around a bug or not would be helpful. If it is a bug, the comment should say something like "FIXME: remove when bug blah is fixed".

274 ↗(On Diff #189222)

I am sorry for marking it as done. I do not know how people work here exactly and how phabricator behaves. I thought the check boxes are handled for everyone individually. Also, if I add a new comment, it is checked by default.

No worries -- that new comments are automatically marked done by default is certainly a surprise to me!

How are you/most people going to use clang-tidy? Do you run it regularly and automatic? Do you expect it to find zero issues once you applied the check? In other words, if you do not want to transform some functions, do you need an option to disable the check for these, so it runs clean on the full source code?

I think it's hard to gauge how most people do anything, really. However, I think there are people who enable certain clang-tidy checks and have them run automatically as part of CI, etc. Those kind of folks may need the ability to silence the diagnostics. We could do this in a few ways (options to control methods not to diagnose, NOLINT comments, etc).

I kind of think we don't need an option for the user to list functions not to transform. They can use NOLINT to cover those situations as a one-off. The only situation where I wonder if everyone is going to want to write NOLINT is for main(). It might make sense to have an option to not check program entrypoints, but there is technically nothing stopping someone from using a trailing return type with a program entrypoint so maybe this option isn't needed at all.

How about we not add any options and if someone files a bug report, we can address it then?

test/clang-tidy/modernize-use-trailing-return-type.cpp
95–97 ↗(On Diff #189222)

I think I'm leaning towards consistently diagnosing (and attempting to fix when possible) everything and we can add options if users ask for them. I was mostly surprised because I'm used to seeing things in extern "C" blocks being C-ish, but that is personal preference more than anything technical.

bernhardmgruber marked 9 inline comments as done.

It took a long while to figure out how to handle certain macro cases. Here is what I came up with:

When tokenizing the source code from the beginning of the function to the function name, I now use clang's Preprocessor to lex these tokens again and expand macros on the way. I analyse the top-level macros if they just contain specifiers or qualifiers and store this information in a ClassifiedToken. When I later try to expand the return type location to include qualifiers, or when I want to remove specifiers from the return type range, I can use this information to also include/reprosition macros which I can regard as qualifiers or specifiers. This currently solves a lot of cases where macros are part of the return type.

Function style macros as part of the return type are not supported, as they are harder to lex and expand. The check currently provides no fixit in these cases.

Other changes:

  • expandIfMacroId() now expands recursively because the SourceLocation might be inside nested macros
  • renamed nonMacroTokensBeforeFunctionName() into classifyTokensBeforeFunctionName()
  • improved some comments
  • overriding registerPPCallbacks to hijack a reference to the preprocessor
  • expanding macro ids in the initial return type source range gotten from the FunctionDecl
bernhardmgruber added inline comments.May 4 2019, 8:26 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
180–184 ↗(On Diff #189993)
#define const const
const int f(void); // Why no fixit?

This can be handled now. As well as e.g.:

#define ATT __attribute__((deprecated))
ATT const int f();
234 ↗(On Diff #189993)

Specifiers at arbitrary locations inside the return type should be supported now.

268 ↗(On Diff #191233)

int static&();
Running keepSpecifiers() on all return types now.

274 ↗(On Diff #189222)

How about we not add any options and if someone files a bug report, we can address it then?

Sounds good to me!

aaron.ballman added inline comments.May 6 2019, 1:20 PM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
85–87 ↗(On Diff #198143)

You can combine these.

88–90 ↗(On Diff #198143)

And this can become return TraverseTypeLoc(...);

95–98 ↗(On Diff #198143)

This can also be simplified into a single return statement rather than an if, but it's less clear to me whether that's an improvement. WDYT?

203 ↗(On Diff #198143)

This should return llvm::None

234 ↗(On Diff #198143)

llvm::None

246 ↗(On Diff #198143)

llvm::None

bernhardmgruber marked 8 inline comments as done.

Fixed small nits suggested by @aaron.ballman. Thanks!

aaron.ballman accepted this revision.May 8 2019, 5:47 AM

Aside from a formatting issue, this LGTM, thank you!

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
93 ↗(On Diff #198531)

80-col limit.

This revision is now accepted and ready to land.May 8 2019, 5:47 AM

@aaron.ballman and @JonasToth: Thank you for the patience and all the feedback! It means a great deal to me to have a patch accepted here!

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
95–98 ↗(On Diff #198143)

Let's simplify it.

203 ↗(On Diff #198143)

I always wondered what the point of llvm::None, std::nullopt or boost::none is. When I write return {}; it looks like i return an empty shell, exactly how I picture an empty optional in my head. That is why I prefer it this way. I will change it of course for this patch, but would you mind giving me a short reason, why llvm::None is preferable here?

@aaron.ballman and @JonasToth: Thank you for the patience and all the feedback! It means a great deal to me to have a patch accepted here!

You're very welcome! Do you need one of us to commit this on your behalf, or do you already have commit privileges?

aaron.ballman added inline comments.May 9 2019, 5:42 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
203 ↗(On Diff #198143)

AFAIK, there's no functional difference between the two and it's more a matter of consistency. Multiple different types can have the notion of a null state, and this allows you to consistently specify that null state across types in an explicit way.

I suppose there might also be a very slight argument for clarity in that a user reading the code with {} could be confused into thinking that is default constructing the contained type rather than creating an empty optional object.

bernhardmgruber marked 2 inline comments as done.May 9 2019, 5:46 AM

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
203 ↗(On Diff #198143)

Thank you for the explanation! I see the point of being more explicit.

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

I've commit for you in r360345, thank you for the patch!

aaron.ballman closed this revision.May 9 2019, 7:46 AM
aaron.ballman reopened this revision.May 9 2019, 8:06 AM

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

I've commit for you in r360345, thank you for the patch!

I had to revert in r360348 as the patch does not pass the testbots: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/48063/steps/test/logs/stdio

When you correct the issues, I'm happy to reapply the patch for you. If you could also fix up the link in the release notes as part of the cleanup, that would be great (it turned out there was a typo -- I've pointed it out in the review).

docs/ReleaseNotes.rst
191–192

These should be modernize-use-trailing-return-type instead (there's a typo).

This revision is now accepted and ready to land.May 9 2019, 8:06 AM
bernhardmgruber updated this revision to Diff 198937.EditedMay 9 2019, 4:13 PM
bernhardmgruber marked 3 inline comments as done.
  • fixed formatting
  • fixed function names in tests
  • added -fexceptions to test arguments
  • fixed typo in release notes
aaron.ballman closed this revision.May 10 2019, 9:26 AM
  • fixed formatting
  • fixed function names in tests
  • added -fexceptions to test arguments
  • fixed typo in release notes

Thanks! I committed in r360438.

Please take a look on PR44206.

@Eugene.Zelenko I tried to find what you refer to by PR44206, but I could not find anything :/ Can you please provide me with a link? Thank you!

@Eugene.Zelenko I tried to find what you refer to by PR44206, but I could not find anything :/ Can you please provide me with a link? Thank you!

See https://bugs.llvm.org/show_bug.cgi?id=44206.