Skip to content

[NFCI][analyzer] Make CallEvent::getState protected #162673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 18, 2025
+24 −11
Merged

[NFCI][analyzer] Make CallEvent::getState protected #162673

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
769a309
[NFCI][analyzer] Make CallEvent::getState protected
NagyDonat Oct 1, 2025
e0d10ba
Shorten and clarify comments
NagyDonat Oct 13, 2025
ca65416
Make the returned ASTContext const
NagyDonat Oct 13, 2025
1d76d59
Satisfy git-clang-format
NagyDonat Oct 13, 2025
dd4a5c7
Clarify TODO commet
NagyDonat Oct 15, 2025
847bb88
Merge branch 'main' into CallEvent-make-getState-protected
NagyDonat Oct 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ class CallEvent {
getExtraInvalidatedValues(ValueList &Values,
RegionAndSymbolInvalidationTraits *ETraits) const {}

/// The state in which the call is being evaluated.
/// CallEvent instances have a reference to a state object instead of storing
/// the argument `SVal`s, the return value (if available), the dynamic type
/// information and similar things as separate data members. However, the
/// existence of this state object is an implementation detail (and perhaps
/// it should be eventually eliminated), so use of this method should be
/// avoided if possible. (The checker callbacks can and should access the
/// canonical state through the CheckerContext.)
const ProgramStateRef &getState() const { return State; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By reading the comment it was not clear to me why it returns a state by const ref.
State objects can never be mutated. Only a new one could be constructed that applies the mutation one desired.

BTW I also feel like a comment should fit into a single line, or maybe just a couple of lines, if the intention is that someone should read it.
If the message can't be expressed in that limited space, it must be chunked, to make it easier to read. Possibly also use Doxygen note or similar sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By reading the comment it was not clear to me why it returns a state by const ref.
State objects can never be mutated. Only a new one could be constructed that applies the mutation one desired.

I just moved this method from the public section to protected without modifying its signature. You're right that there is no reason to take the state by (const) ref, so I'll update it.

BTW I also feel like a comment should fit into a single line, or maybe just a couple of lines, if the intention is that someone should read it.

I'll try to reduce the length of the comment -- although personally I read and often prefer long and explicit comments.

Copy link
Contributor

@steakhal steakhal Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So the discouragement is to avoid more dependence on using the State. IMO. if that is a concern, we could easily swap that State for a reference to the ASTContext if we decide to drop that State field.
To me, the comment does not pull its weight, and probably confusing (just like it did confuse me).

Probably any comments about the State filed should go to the field declaration, and not to this getter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So the discouragement is to avoid more dependence on using the State. IMO. if that is a concern, we could easily swap that State for a reference to the ASTContext if we decide to drop that State field. To me, the comment does not pull its weight, and probably confusing (just like it did confuse me).

Is this still relevant after my commit e0d10ba or did I manage to clarify it?

You are right that "we could easily swap that State for a reference to the ASTContext if we decide to drop that State field" and therefore we don't strictly need the discouragement before getASTContext() [which is btw the other big comment block] -- but I still feel that "can provide an ASTContext" is not a "natural" responsibility for a call event object, so I would prefer to keep this discouragement.

Probably any comments about the State filed should go to the field declaration, and not to this getter.

The warning which highlights the use of the state object belongs to the getter, because a developer who wants to use getState() perhaps looks at its declaration and sees the warning, but almost surely won't look at the field declaration. (Although perhaps we should rename getState to getStateUnsafe to highlight that there is something fishy going on, because otherwise developers happily autocomplete getState without seeing the warning comment.)

You're right that the TODO note for replacing the State field with smaller individual fields would logically belong to the field declaration, but it is also closely related to the warning comment, so I don't want to separate it from there.


public:
CallEvent &operator=(const CallEvent &) = delete;
virtual ~CallEvent() = default;
Expand All @@ -231,8 +241,14 @@ class CallEvent {
}
void setForeign(bool B) const { Foreign = B; }

/// The state in which the call is being evaluated.
const ProgramStateRef &getState() const { return State; }
/// Returns the ASTContext which is (indirectly) associated with this call
/// event. This method is exposed for the convenience of a few checkers that
/// need the AST context in functions that take a CallEvent as argument. For
/// the sake of clarity prefer to get the ASTContext from more "natural"
/// sources (e.g. the CheckerContext) instead of using this method!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think preferring the ASTContext of the CheckerContext is better?
I was expecting no comments at this member function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think preferring the ASTContext of the CheckerContext is better?

Because the fact that CallEvent can easily provide an ASTContext is just an accidental consequence of the current implementation. This is not a natural role for a call event object, and in fact, there are vague plans to avoid storing a state within the CallEvent (suggested at #160707 (comment)) and in that case we would probably eliminate this method.

ASTContext &getASTContext() const {
return getState()->getStateManager().getContext();
}

/// The context in which the call is being evaluated.
const LocationContext *getLocationContext() const { return LCtx; }
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class RAIIMutexDescriptor {
// this function is called instead of early returning it. To avoid this, a
// bool variable (IdentifierInfoInitialized) is used and the function will
// be run only once.
const auto &ASTCtx = Call.getState()->getStateManager().getContext();
const auto &ASTCtx = Call.getASTContext();
Guard = &ASTCtx.Idents.get(GuardName);
}
}
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,7 +929,7 @@ ObjCDeallocChecker::getValueReleasedByNillingOut(const ObjCMethodCall &M,
SVal Arg = M.getArgSVal(0);
ProgramStateRef notNilState, nilState;
std::tie(notNilState, nilState) =
M.getState()->assume(Arg.castAs<DefinedOrUnknownSVal>());
C.getState()->assume(Arg.castAs<DefinedOrUnknownSVal>());
if (!(nilState && !notNilState))
return nullptr;

Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ ObjCSuperDeallocChecker::isSuperDeallocMessage(const ObjCMethodCall &M) const {
if (M.getOriginExpr()->getReceiverKind() != ObjCMessageExpr::SuperInstance)
return false;

ASTContext &Ctx = M.getState()->getStateManager().getContext();
ASTContext &Ctx = M.getASTContext();
initIdentifierInfoAndSelectors(Ctx);

return M.getSelector() == SELdealloc;
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,13 +211,13 @@ class StdVariantChecker : public Checker<eval::Call, check::RegionChanges> {
if (!DefaultType)
return;

ProgramStateRef State = ConstructorCall->getState();
ProgramStateRef State = C.getState();
State = State->set<VariantHeldTypeMap>(ThisMemRegion, *DefaultType);
C.addTransition(State);
}

bool handleStdGetCall(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = Call.getState();
ProgramStateRef State = C.getState();

const auto &ArgType = Call.getArgSVal(0)
.getType(C.getASTContext())
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Checkers/TaggedUnionModeling.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ removeInformationStoredForDeadInstances(const CallEvent &Call,
template <class TypeMap>
void handleConstructorAndAssignment(const CallEvent &Call, CheckerContext &C,
SVal ThisSVal) {
ProgramStateRef State = Call.getState();
ProgramStateRef State = C.getState();

if (!State)
return;
Expand Down