This check verifies the safety of access to std::optional and related
types (including absl::optional). It is based on a corresponding Clang
Dataflow Analysis, which does most of the work. This check merely runs it and
converts its findings into diagnostics.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
86 | Please don't use auto if type is not spelled in same statement or iterator. | |
91 | Ditto. | |
clang-tools-extra/docs/ReleaseNotes.rst | ||
119 | Please add description. Hint: should be same as first statement in documentation. | |
clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst | ||
7 | Please do that. |
Thanks, Eugene -- I'm actually still working on this, filling out the FIXMEs, etc. I uploaded it with arc as a draft and its clearly marked as such. Is there something more I should have done to preven it from being sent out?
Most checks in Clang Tidy will run relatively quickly as they usually can do most/all of their work in a single traversal. I wonder whether flow sensitive checks will prove to be a bit slower. I think adding slower checks to Tidy is fine, but it would be nice to properly set the expectations to the user (e.g. if an IDE is running Tidy in the background it might want to opt out from flow sensitive checks if they turn out to be slow). In case the performance is good, I think it should be fine as is. Otherwise I wonder if we want to add something like a tag to mark flow sensitive checks and give an option to turn them off.
Excellent point. @aaron.ballman is there a mechanism for this?
I can check on the current performance, but I think this would be good to know regardless of the outcome, given that performance is subject to change and we expect to develop other flow-sensitive checks.
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
39 | I am a bit afraid that we have two places to update the list of supported optional types and they can get out of sync. I wonder whether creating a function in clang/Analysis/FlowSensitive/Models/UncheckedOptionalUseModel.h to return a matcher that matches the supported optional types is a better approach. | |
50 | If we have a CXXConstructorDecl where both the initializer and the body has calls to optional types, i.e. both matchers would match, would we process the body of that twice? | |
61 | Nit: Redundant braces. | |
93 | Is there a way in the future to include either the name of the optional or a source range of the access? This can be confusing when we have multiple optionals in the same line. The of course, the column information helps. But the more visual guides we have the better :) |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
39 | Yes, it is a better approach. :) Done. | |
50 | Yes, we would. Good catch. Fixed. | |
93 | Yes -- we have access to the object expression at the time of detection. So both are reasonable to include. We just need to extend the SourceLocationsLattice to accomodate. Added a FIXME in the model. |
Overall this looks good to me. However, I think this might not use the full potential of the check itself. With the information that the dataflow framework have it could distinguish between potentially unsafe accesses and provably unsafe accesses depending on whether the has_value property is constrained to be false. From the user point of view, it would be nice to emit different warning messages for the above two cases. This can help to gradually introduce this check to a larger codebase and focus on the higher severity diagnostics first.
clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst | ||
---|---|---|
30 | I wonder if it would be easier to read if we had two top level categories, one for safe and one for unsafe accesses instead of switching back and forth between examples. |
clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst | ||
---|---|---|
30 | Sounds good, done. Let me know if that's what you had in mind. |
clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst | ||
---|---|---|
30 | Yup, this is exactly what I had in mind, thanks! |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
84 | Based on my reading, it is a rare, but possible condition. Basically, we need code where the exit block is unreachable, which I believe can happen in weird cases like: while(true) {...} https://godbolt.org/z/rfEnfaWTv -- notice the lack of predecessors for the exit block. See the code here, which follows the ordering of the blocks and doesn't force blocks to be processed: |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
84 | Interesting. Since we already have optionals in the vector, I assumed we will always have matching size. I think we might want to change this so there is only one way for the analysis to not provide a state for a basic block to make this a bit less confusing, |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
84 | Actually, in the linked code I see BlockStates.resize(CFCtx.getCFG().size(), llvm::None);. So I would expect the size to be always right with possibly some Nones for the nodes that were not processed. |
Agreed. I've filed this in our issue tracker (which I'm still planning to port over to github...).
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
84 |
Ah, my mistake! I thought resize only allocated the space. #TIL Changed to an assert. Thanks. |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
84 | But this discussion shed light on an interesting detail. If the exit block is unreachable, we will not diagnose the unsafe accesses. I wonder if this worth a FIXME. |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
40–46 | Do we really need all these using declarations? There seems to be one reference for each of these types. I think we can simply qualify the references. | |
55 | Call this Env to disambiguate from the name of the type. | |
84 | The documentation of runDataflowAnalysis already hints that the size of the vector matches the size of the CFG. Do you think we should make this more clear? | |
88 | Let's add a constant for this so that we can reuse it in UncheckedOptionalAccessCheck::check. | |
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h | ||
70 | ||
clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp | ||
25 | Call this checked_access? | |
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h | ||
39 | ||
40 | This should be a class member, no? clang::dataflow::optionalClass seems underspecified. | |
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | ||
237 |
Thanks for the review!
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
40–46 | No, we don't need them, but IMO they clarify the code. We're really heavy with the types, given that auto is discouraged, so I think pulling these out improves readability in some critical places (lines 56 and 63). I figured I'd do all for consistency. I've removed those not related to lines 56 and 63, or used more than once. WDYT? | |
84 |
Yes to both. I'll send a separate patch on that header with the doc changes. |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
84 | On second thought, the FIXME is for the check, not the framework, so I've added it here. I'll still send the comment tweak separately. |
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | ||
---|---|---|
40–46 | Ah, I see, we're using all of these types in a single line :D Perhaps std::vector<Optional<DataflowAnalysisState<T>>> could be encapsulated in a DataflowResult<T> type or something like that. I suggest we keep the patch as it currently is and consider such a change separately. |
Just realized we left this open. How about a disclaimer at the top of the doc (.rst) file noting the potential cost? Anyone using clang-tidy should be explicitly configuring which checks to run, so that may be sufficient. If we want to allow users to enable/disable flow-sensitive checks across the board, though, it seems like we would need to add a new option to ClangTidyOptions (https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidyOptions.h#L50).
I am a bit afraid that we have two places to update the list of supported optional types and they can get out of sync. I wonder whether creating a function in clang/Analysis/FlowSensitive/Models/UncheckedOptionalUseModel.h to return a matcher that matches the supported optional types is a better approach.