This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add bugprone-optional-value-conversion check
ClosedPublic

Authored by PiotrZSL on Mar 31 2023, 1:57 PM.

Details

Summary

Detects potentially unintentional and redundant conversions where a value is
extracted from an optional-like type and then used to create a new instance of
the same optional-like type.

Diff Detail

Event Timeline

PiotrZSL created this revision.Mar 31 2023, 1:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL requested review of this revision.Mar 31 2023, 1:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 1:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
46

Unintended quote in value.

There are 37 findings in llvm repository:

  1. clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:96:12: warning: conversion from 'std::optional<bool>' into 'bool' and back into 'std::optional<bool>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  2. clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:65:12: warning: conversion from 'std::optional<std::basic_string<char>>' into 'std::basic_string<char>' and back into 'std::optional<std::basic_string<char>>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  3. clang/lib/AST/Interp/Program.cpp:173:12: warning: conversion from 'std::optional<unsigned int>' into 'unsigned int' and back into 'std::optional<unsigned int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  4. clang/lib/Analysis/PathDiagnostic.cpp:323:14: warning: conversion from 'std::optional<bool>' into 'bool' and back into 'std::optional<bool>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  5. clang/lib/Frontend/Rewrite/InclusionRewriter.cpp:253:7: warning: conversion from 'std::optional<llvm::MemoryBufferRef>' into 'llvm::MemoryBufferRef' and back into 'std::optional<llvm::MemoryBufferRef>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  6. clang/lib/Sema/Scope.cpp:190:25: warning: conversion from 'std::optional<clang::VarDecl *>' into 'clang::VarDecl *' and back into 'std::optional<clang::VarDecl *>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  7. clang/lib/Sema/TreeTransform.h:13915:9: warning: conversion from 'std::optional<unsigned int>' into 'unsigned int' and back into 'std::optional<unsigned int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  8. clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:440:16: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  9. clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp:125:10: warning: conversion from 'std::optional<bool>' into 'bool' and back into 'std::optional<bool>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  10. clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1573:10: warning: conversion from 'std::optional<clang::ento::nonloc::LazyCompoundVal>' into 'clang::ento::nonloc::LazyCompoundVal' and back into 'std::optional<clang::ento::nonloc::LazyCompoundVal>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  11. clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1784:12: warning: conversion from 'std::optional<clang::ento::SVal>' into 'clang::ento::SVal' and back into 'std::optional<clang::ento::SVal>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  12. clang/lib/Tooling/InterpolatingCompilationDatabase.cpp:189:16: warning: conversion from 'std::optional<clang::driver::types::ID>' into 'clang::driver::types::ID' and back into 'std::optional<clang::driver::types::ID>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  13. lldb/source/Breakpoint/BreakpointResolver.cpp:233:33: warning: conversion from 'std::optional<unsigned short>' into 'unsigned short' and back into 'std::optional<unsigned short>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  14. lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1362:53: warning: conversion from 'std::optional<unsigned long>' into 'unsigned long' and back into 'std::optional<unsigned long>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  15. llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:4119:12: warning: conversion from 'std::optional<LiveDebugValues::ValueIDNum>' into 'LiveDebugValues::ValueIDNum' and back into 'std::optional<LiveDebugValues::ValueIDNum>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  16. llvm/lib/DWARFLinker/DWARFLinker.cpp:1191:14: warning: conversion from 'std::optional<unsigned long>' into 'unsigned long' and back into 'std::optional<unsigned long>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  17. llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1305:31: warning: conversion from 'std::optional<long>' into 'long' and back into 'std::optional<long>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  18. llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1322:62: warning: conversion from 'std::optional<llvm::DWARFFormValue>' into 'llvm::DWARFFormValue' and back into 'std::optional<llvm::DWARFFormValue>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  19. llvm/lib/DebugInfo/Symbolize/Markup.cpp:67:14: warning: conversion from 'std::optional<llvm::symbolize::MarkupNode>' into 'llvm::symbolize::MarkupNode' and back into 'std::optional<llvm::symbolize::MarkupNode>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  20. llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:425:13: warning: conversion from 'std::optional<llvm::raw_ostream::Colors>' into 'llvm::raw_ostream::Colors' and back into 'std::optional<llvm::raw_ostream::Colors>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  21. llvm/lib/InterfaceStub/IFSHandler.cpp:231:24: warning: conversion from 'std::optional<unsigned short>' into 'unsigned short' and back into 'std::optional<unsigned short>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  22. llvm/lib/InterfaceStub/IFSHandler.cpp:239:30: warning: conversion from 'std::optional<llvm::ifs::IFSEndiannessType>' into 'llvm::ifs::IFSEndiannessType' and back into 'std::optional<llvm::ifs::IFSEndiannessType>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  23. llvm/lib/InterfaceStub/IFSHandler.cpp:246:28: warning: conversion from 'std::optional<llvm::ifs::IFSBitWidthType>' into 'llvm::ifs::IFSBitWidthType' and back into 'std::optional<llvm::ifs::IFSBitWidthType>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  24. llvm/lib/LTO/LTOBackend.cpp:214:18: warning: conversion from 'std::optional<llvm::Reloc::Model>' into 'llvm::Reloc::Model' and back into 'std::optional<llvm::Reloc::Model>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  25. llvm/lib/LTO/LTOBackend.cpp:221:17: warning: conversion from 'std::optional<llvm::CodeModel::Model>' into 'llvm::CodeModel::Model' and back into 'std::optional<llvm::CodeModel::Model>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  26. llvm/lib/Remarks/BitstreamRemarkParser.cpp:563:17: warning: conversion from 'std::optional<unsigned long>' into 'unsigned long' and back into 'std::optional<unsigned long>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  27. llvm/lib/Support/UnicodeNameToCodepoint.cpp:382:12: warning: conversion from 'std::optional<char32_t>' into 'char32_t' and back into 'std::optional<char32_t>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  28. llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:1028:12: warning: conversion from 'std::optional<llvm::Value *>' into 'llvm::Value *' and back into 'std::optional<llvm::Value *>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  29. llvm/lib/Transforms/Utils/SimplifyCFG.cpp:4651:33: warning: conversion from 'std::optional<unsigned int>' into 'unsigned int' and back into 'std::optional<unsigned int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  30. llvm/lib/Transforms/Utils/ValueMapper.cpp:576:12: warning: conversion from 'std::optional<llvm::Metadata *>' into 'llvm::Metadata *' and back into 'std::optional<llvm::Metadata *>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  31. llvm/lib/Transforms/Utils/ValueMapper.cpp:615:12: warning: conversion from 'std::optional<llvm::Metadata *>' into 'llvm::Metadata *' and back into 'std::optional<llvm::Metadata *>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  32. llvm/lib/Transforms/Utils/ValueMapper.cpp:832:12: warning: conversion from 'std::optional<llvm::Metadata *>' into 'llvm::Metadata *' and back into 'std::optional<llvm::Metadata *>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  33. llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:435:14: warning: conversion from 'std::optional<unsigned int>' into 'unsigned int' and back into 'std::optional<unsigned int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  34. llvm/tools/llvm-lto2/llvm-lto2.cpp:264:23: warning: conversion from 'std::optional<llvm::Reloc::Model>' into 'llvm::Reloc::Model' and back into 'std::optional<llvm::Reloc::Model>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  35. llvm/tools/llvm-profgen/ProfiledBinary.cpp:104:14: warning: conversion from 'std::optional<unsigned int>' into 'unsigned int' and back into 'std::optional<unsigned int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  36. llvm/tools/llvm-profgen/ProfiledBinary.cpp:118:16: warning: conversion from 'std::optional<unsigned int>' into 'unsigned int' and back into 'std::optional<unsigned int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
  37. llvm/utils/TableGen/GlobalISelEmitter.cpp:5217:12: warning: conversion from 'std::optional<const llvm::CodeGenRegisterClass *>' into 'const llvm::CodeGenRegisterClass *' and back into 'std::optional<const llvm::CodeGenRegisterClass *>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
PiotrZSL marked an inline comment as done.Mar 31 2023, 2:30 PM

Have you thought of the possibility of handling this case

takeOptionalValue(std::move(*param));
clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
35

Any reason for this limitation, given in most cases the fix is to just remove the * or .value() call

clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
17

It'd be good to have tests demonstating boost::optional and absl::optional as well as the (non standard) get accessor

As for takeOptionalValue(std::move(*param));, it could be added as an configuration option for PassthroughFunctions or UtilityFunctions. I can thing about that, that should be easy to implement.

clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
35

Sometimes problem is not with optional but with called function that take optional when it could take value.
At least for such issues I run, still probably could be safe to introduce such auto-fix to just remove */.value().
But then question would be if it should be bugprone or readability check.
For me like 10% of these issues show some possible bug around that code.

clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
17

I added them to config just to add them, as they should work, I could add some tests for them, but behavior should be same...

As for takeOptionalValue(std::move(*param));, it could be added as an configuration option for PassthroughFunctions or UtilityFunctions. I can thing about that, that should be easy to implement.

That's overcomplicating things, just look for std::move and you'll cover 99.9% of instances in real world code.

clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
35

That's a good point, if they are hiding a bug we don't want to auto fix it.
Perhaps the fix should be added as a note

note: Remove the call to '%FuncName%' to silence this warning
note: Remove '*' to silence this warning
clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
17

On the subject of boost(unsure about absl as I've never used that library)
Would you ever add support for T& boost::get(boost::optional<T> &) or its const ref cousin?

38

What happens with this pathological case
takeOptionalValue(param.operator*());
Looks like it isn't being handled, should probably handle(and test) for it

PiotrZSL updated this revision to Diff 512559.Apr 11 2023, 1:08 PM
PiotrZSL marked 3 inline comments as done.

Add fixes, add suport for std:move, fixed operator*() calls, added more tests

PiotrZSL planned changes to this revision.Apr 11 2023, 1:09 PM

TODO: Fix pointers, Add more tests, consider free standing value extraction functions like boost::get

PiotrZSL marked 2 inline comments as done.Apr 20 2023, 2:03 PM
PiotrZSL added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
17

No, I'm not planing to add support for boost::get. I added basic support for boost and absl only because it's cheap, and interfaces are similar. Main target is std::optional. And configuration is provider just because for example in project that I work for we got custom optional implementation.
Added info about limited support for non-standard optional.

PiotrZSL updated this revision to Diff 515475.Apr 20 2023, 2:03 PM
PiotrZSL marked an inline comment as done.

Added support for ->, added more tests.

Ready for review.

PiotrZSL updated this revision to Diff 515476.Apr 20 2023, 2:05 PM

Marked check as one that provides fixes.

PiotrZSL updated this revision to Diff 545372.Jul 29 2023, 7:16 AM

Rebase + Ping

xgupta added a subscriber: xgupta.Jul 30 2023, 10:41 AM
xgupta added inline comments.
clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
62

Any good reason to choose "fun" as the matcher name?

clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
33

Can we also write - A better approach would be to directly pass opt to the print function without extracting its value:
print(opt)

PiotrZSL added inline comments.Jul 30 2023, 12:11 PM
clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
62

shortcut from function, as its matcher for a cxxMemberCallExpr i will change it to for example "call"

clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
33

Yes I will add such thing...

PiotrZSL updated this revision to Diff 545464.Jul 30 2023, 12:36 PM

Review comments

xgupta accepted this revision.Jul 30 2023, 6:02 PM

LGTM, Thanks!

This revision is now accepted and ready to land.Jul 30 2023, 6:02 PM