This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement type-based check for `gsl::owner`
ClosedPublic

Authored by JonasToth on Aug 5 2017, 6:31 AM.

Details

Summary

This check implements the typebased semantic of gsl::owner.
Meaning, that

  • only gsl::owner is allowed to get deleted
  • new expression must be assigned to gsl::owner
  • function calls that expect gsl::owner as argument, must get either an owner or a newly created and recognized resource (in the moment only newed memory)
  • assignment to gsl::owner must be either a resource or another owner
  • functions returning an gsl::owner are considered as factories, and their result must be assigned to an gsl::owner
  • classes that have an gsl::owner-member must declare a non-default destructor

There are some problems that occur when typededuction is in place.
For example auto Var = function_that_returns_owner(); the type of Var will not be
an gsl::owner. This case is catched, and explicitly noted.

But cases like fully templated functions

template <typename T> 
void f(T t) { delete t; }
// ...
f(gsl::owner<int*>(new int(42)));

Will created false positive (the deletion is problematic), since the type deduction
removes the wrapping typeAlias.

Please give your comments :)

Event Timeline

JonasToth created this revision.Aug 5 2017, 6:31 AM
JonasToth updated this revision to Diff 109866.Aug 5 2017, 6:34 AM
  • [Misc] remove not implemented c function stuff

Running this check on clang with llvm/tools/clang$ run-clang-tidy.py -j8 -checks=-*,cppcoreguidelines-owning-memory * 2>&1 1>~/OwnerClangOutput.txt

The result is cropped, since it get repetetive.

JonasToth updated this revision to Diff 109867.Aug 5 2017, 6:40 AM
  • I broke the diff, try to restore
JonasToth updated this revision to Diff 109868.Aug 5 2017, 7:09 AM
  • [Fix] codeblocks in documentation needed empty line
Eugene.Zelenko added inline comments.
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
16

Please remove this line.

docs/ReleaseNotes.rst
60–78

Please use previous version Release Notes as example for adding new check.

docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst
7

Please use `` for language constructs. Same in other places.

JonasToth updated this revision to Diff 109872.Aug 5 2017, 8:03 AM
JonasToth marked 3 inline comments as done.
  • [Fix] review issues by eugene solved

mark as done by eugene

JonasToth updated this revision to Diff 109875.Aug 5 2017, 8:11 AM
  • [Misc] fix new line in Release Note
aaron.ballman added inline comments.Aug 8 2017, 7:51 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
30

Can elide the braces.

33

Variable names should start with an uppercase letter (here and elsewhere).

40

Can remove the comma and add a hyphen to non-owners.

56

non-owners

76

Can remove the first comma.

84

Can remove the first comma.

94

Can remove the first comma.

102

Can remove the first comma.

109

Can remove the first comma.

118

Can remove the first comma.

129

Please split this function up into smaller functions that check only the individual components.

clang-tidy/cppcoreguidelines/OwningMemoryCheck.h
20

usecases -> use cases

JonasToth updated this revision to Diff 110547.Aug 10 2017, 3:53 AM
JonasToth marked 10 inline comments as done.

[Misc] address issues from aaron, especially refactor the big check function

JonasToth marked an inline comment as done.Aug 10 2017, 3:55 AM
JonasToth marked an inline comment as done.Aug 10 2017, 3:57 AM
JonasToth updated this revision to Diff 110548.Aug 10 2017, 3:57 AM

[Fix] typo

[Misc] remove unneccessary stuff

aaron.ballman added inline comments.Aug 11 2017, 5:45 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
150

Can DeletedVariable ever be null if DeleteStmt is non-null? I don't think it can, so perhaps the best approach is to gate on DeleteStmt and remove the assert.

154

"use RAII" is not particularly helpful. I think this means you should use a smart pointer, no? If so, I'd recommend: "deleting a pointer through a type that is not marked 'gsl::owner'; consider using a smart pointer instead" or something along those lines.

155–156

Can call DeletedVariable->getSourceRange() instead (same applies elsewhere).

158

No else after a return (same applies elsewhere).

169

I'm having a bit of a hard time understanding the guidance in this warning. How about: "argument is expected to be of type 'gsl::owner<>'; got %0" and pass in the type of the argument?

I think something needs to be done to better-define "recognized resource" before it's used as a term of art in the diagnostics; this applies elsewhere as well.

189

Same comments here as above about "owner" and "recognized resource". I think you want to talk about gsl::owner<> where you use "owner" and drop "recognized resource" (because such a resource returns a gsl::owner<>-flagged type, if I understand properly).

JonasToth added inline comments.Aug 11 2017, 5:59 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
150

I think that both will be matched simultaneously, but i was unsure, therefor i programmed it defensive.
I can change it if you want.

154

in this case RAII means smartpointers, but in general gsl::owner could be file handles, sockets and whatever else.

i will adjust the warning to mention smartpointers.

169

I used the term 'recognized resource' since FILE* would be a resource as well, but the there is no way the C standard function could return an gsl::owner<>.

I plan to enhance this check, to allow of lists of C/C++ functions that are known to create a resource, but can not be changed.
The effect would be like in the case of new - whenever such a function is called, the result must be assigned to an gsl::owner.
Furthermore the CppCoreGuidelines usually talk about 'resources', so the wording is somewhat similar.

same applies to next comment

JonasToth updated this revision to Diff 110730.Aug 11 2017, 8:10 AM
JonasToth marked 6 inline comments as done.
  • order release notes
  • address aarons comments
  • adjust diagnostic for function arguments
JonasToth marked an inline comment as done.Aug 11 2017, 8:12 AM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
189

ideally every created resource would be flagged as gsl::owner<>, but for example fopen, malloc and friends can't, but would benefit the most from such a check (+ flow analysis if the owner gets consumed on every path)

JonasToth updated this revision to Diff 110731.Aug 11 2017, 8:17 AM

remove manual source range creation, where i forgot it before

aaron.ballman added inline comments.Aug 11 2017, 11:46 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
168

You can pass the QualType directly instead of calling getAsString() on it.

185

No need to check against nullptr explicitly (here and elsewhere).

189

The use case certainly makes sense, but that doesn't make the diagnostic any more clear to the user. Perhaps: "expected assignment source to be of type 'gsl::owner<>'; got %0"? It's not perfect because it doesn't mention "recognized resource", but that can be improved in the follow-up patch that defines those resources.

The wording should be similar for the below cases as well.

192

No else after a return. Same applies elsewhere.

aaron.ballman added inline comments.Aug 11 2017, 11:46 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
209

It's hard to understand how this differs from handleAssignmentAndInit(); might need a more descriptive name.

231

Are you intending to address this in this patch?

Also, it should probably be "FIXME: " (with the colon).

JonasToth marked 5 inline comments as done.Aug 11 2017, 1:25 PM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
231

no not in this patch directly, but i plan a follow up after getting this one done.

JonasToth updated this revision to Diff 110799.Aug 11 2017, 1:25 PM
JonasToth marked an inline comment as done.

address review comments from aaron

JonasToth marked an inline comment as done.Aug 11 2017, 1:26 PM
aaron.ballman added inline comments.Aug 16 2017, 6:48 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
192

Please remove the use of CheckApplied. The early returns were fine, it's the complex if/else logic that should be modified. e.g.,

if (OwnerAssignment) {
  <stuff>
  return true;
}

if (OwnerInitialization) {
  <stuff>
  return true;
}
...
return false;
JonasToth marked 2 inline comments as done.Aug 23 2017, 5:18 AM
This comment was removed by JonasToth.
JonasToth updated this revision to Diff 112336.Aug 23 2017, 5:19 AM
  • test case fixed expected warning
  • address review comments from aaron
JonasToth updated this revision to Diff 112363.Aug 23 2017, 7:08 AM
JonasToth marked 3 inline comments as done.
  • adjust the diagnostics to include the types

i think the diagnostics are better now, i added the expected and received type everywhere i could.

JonasToth updated this revision to Diff 112364.Aug 23 2017, 7:12 AM

remove unnecessary outcommented lines

aaron.ballman added inline comments.Aug 25 2017, 9:23 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
203

Can remove the comma.

240

s/non owner/non-owner

257

s/non owner/non-owner

293

s/an declared/a declared

Also, you should either add punctuation or remove the capital M in Match.

295

Instead of diagnosing this on the class, why not diagnose the individual offending members (and then you don't need a note, either)? (Note, that may change the suggested diagnostic wording below).

296

This diagnostic isn't grammatically correct; how about: class with a member variable of type 'gsl::owner<>' but no destructor to release the owned resource?

JonasToth added inline comments.Aug 26 2017, 2:37 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
295

yes, this is probably better.
i plan to add deeper analysis, that gsl::owner<> will always be released or moved.
That would check, if destructors do this correctly, so offending the member is the better way.

JonasToth updated this revision to Diff 113217.Aug 30 2017, 2:19 AM
JonasToth marked 5 inline comments as done.
  • fix spelling and grammar errors
  • adjust unit test to new diagnostics

addressed some issues, not all yet

JonasToth updated this revision to Diff 113265.Aug 30 2017, 8:00 AM
  • adjusted the diagnostic for member variables
JonasToth marked 2 inline comments as done.Aug 30 2017, 8:01 AM
JonasToth added inline comments.Aug 30 2017, 8:43 AM
docs/clang-tidy/checks/cppcoreguidelines-owning-memory.rst
12

s/an/a/

18

s/it/It/

28

s/caught/checked/

98

s/an/a/

132

therefore ... therefore

docs/clang-tidy/checks/hicpp-signed-bitwise.rst
1 ↗(On Diff #113265)

remove from diff

docs/clang-tidy/checks/list.rst
71

remove from diff

test/clang-tidy/hicpp-signed-bitwise.cpp
1 ↗(On Diff #113265)

remove from diff

JonasToth updated this revision to Diff 113272.Aug 30 2017, 8:47 AM
JonasToth marked 5 inline comments as done.
  • fix typos and bad language

language in the docuementation improved

JonasToth marked an inline comment as done.Aug 30 2017, 8:48 AM
JonasToth marked 2 inline comments as done.
aaron.ballman added inline comments.Sep 8 2017, 6:58 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
141

Can assert CheckExecuted and drop the explicit equality test.

142

did execute, -> executed;

207

initializing -> intialization

304–306

Please remove the commented-out lines.

test/clang-tidy/cppcoreguidelines-owning-memory.cpp
40

This diagnostic confuses me -- there's no gsl::owner<> involved anywhere; am I missing something?

JonasToth added inline comments.Sep 8 2017, 11:57 AM
test/clang-tidy/cppcoreguidelines-owning-memory.cpp
40

owner<> is not involved, but the guidelines say, that new must be assigned to an owner.

This is in line with the resource semantics. Everything that creates an resource, that must be released (no RAII available) shall be annotated.

The diagnostic is bad, though.

Returning a newly created resource from function 'functionname', without declaring it as 'gsl::owner<>'; type is '...'

aaron.ballman added inline comments.Sep 8 2017, 1:30 PM
test/clang-tidy/cppcoreguidelines-owning-memory.cpp
40

Okay, that makes more sense to me. I don't think the name of the function helps all that much in the diagnostic, however. What about:

"returning a newly created resource of type %0 from a function whose return type is not 'gsl::owner<>'"

JonasToth updated this revision to Diff 114483.Sep 9 2017, 5:39 AM
JonasToth marked 4 inline comments as done.
  • Merge branch 'master'
  • mention current problems with typedefs of owner<> and adjust test cases
  • Adressed some review comments.
  • Added testcases, where the owner<> annotation might be hidden by a typedef or using, like int using heap_int = gsl::owner<int*>;

The check is currently not able to resolve a typedef to owner<> correctly, and my knowledge in clang is to limited to find exact reason and/or a workaround.
I tried to match the type with hasCanonicalType, but i guess that will remove the owner<> alias and therefore be not an option.
The basic testcase for typedef should stay in the code with the FIXME, since there is probably a way around.

I am thinking of a matcher like IsTypedefToOwner, that looks through one typedef after another recursively until it finds an owner<>, but later and probably with a different solution.

The typedef issue is in the following lines, just as shortcut since the patch is quite big:
testcase: 200-225

The issue is mentioned in the docs as well.

test/clang-tidy/cppcoreguidelines-owning-memory.cpp
40

There is a minor issue with the diagnostic in general, since it is emitted for values of type gsl::owner<> and values that are known to be an owner like new int(42).

There is no easy way to distinguish between a recognized resource or an annotated resource, without complicating the matchers (what i dont want, since there is already a lot happening).

Mixing both cases in the diagnostic would help, but go in the direction of recognized resource, that was decided against earlier.

Would the following modification be acceptable as well?
returning a newly created resource of type %0 or value of type 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
or
returning a newly created resource of type %0 or value of type 'gsl::owner<>' without annotating the return type of the function as 'gsl::owner<>'.

This general problem holds true for other cases, since i want to match for IsConsideredOwner, which wraps cases like new, functions returning owner<> and variables of type owner<>.
I want to expand this further to functions that should return owner<> but can't, like malloc.
Splitting up the matchers instead of using IsConsideredOwner would be a burden including a lot of code duplication.

aaron.ballman added inline comments.Sep 12 2017, 8:18 AM
clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
142

Non -> None

295

Missing a full stop at the end of the sentence.

test/clang-tidy/cppcoreguidelines-owning-memory.cpp
40

Would the following modification be acceptable as well?
returning a newly created resource of type %0 or value of type 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'

I would rephrase slightly to:

returning a newly created resource of type %0 or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'

JonasToth marked 7 inline comments as done.Sep 12 2017, 8:41 AM
JonasToth updated this revision to Diff 114848.Sep 12 2017, 8:41 AM
  • address review comments, especially improve diagnostic for return types, that should be owner but arent
aaron.ballman accepted this revision.Sep 12 2017, 8:53 AM

LGTM, with a few last typos.

test/clang-tidy/cppcoreguidelines-owning-memory.cpp
355

typededuction -> type deduction

(here and elsewhere)

373

harmfull -> harmful

375

nonowner -> non-owner

(here and elsewhere)

This revision is now accepted and ready to land.Sep 12 2017, 8:53 AM
JonasToth marked 3 inline comments as done.Sep 12 2017, 8:56 AM
JonasToth updated this revision to Diff 114849.Sep 12 2017, 8:56 AM
  • fix typos
JonasToth updated this revision to Diff 114851.Sep 12 2017, 9:12 AM
  • fix, that deleted destructors are caught as problem as well
  • clarify one comment, duplicated avoid was misleading
aaron.ballman accepted this revision.Sep 12 2017, 9:15 AM

Good catch on the deleted constructor -- LGTM still!

JonasToth closed this revision.Sep 12 2017, 9:17 AM
JonasToth reopened this revision.Sep 12 2017, 11:17 AM

This Patch broke the buildbot for vs-2015.
I will revert, when i figured out how to do this in svn :/

It does not emit a warning for line 311:
here the log

C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\stage1\tools\clang\tools\extra\test\clang-tidy\Output\cppcoreguidelines-owning-memory.cpp.tmp.cpp:290:3: note: type deduction did not result in an owner
C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\stage1\tools\clang\tools\extra\test\clang-tidy\Output\cppcoreguidelines-owning-memory.cpp.tmp.cpp:297:3: warning: expected assignment source to be of type 'gsl::owner<>'; got 'ArbitraryClass *' [cppcoreguidelines-owning-memory]
Owner3 = &A; // Bad, since assignment of non-owner to owner
^
C:\b\slave\clang-x86-windows-msvc2015\clang-x86-windows-msvc2015\stage1\tools\clang\tools\extra\test\clang-tidy\Output\cppcoreguidelines-owning-memory.cpp.tmp.cpp:343:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'int *' [cppcoreguidelines-owning-memory]

gsl::owner<int *> OwningPtr = Array1.data(); // Bad, since it does not return the owner

I have no idea how to debug this, since i dont have a visual studio installation, neither do i have windows. What could it be?

This revision is now accepted and ready to land.Sep 12 2017, 11:17 AM
  • make matcher for ctor initialization more readable

Recommited with fix in rL313067

JonasToth closed this revision.Sep 13 2017, 12:53 AM