Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
87

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