How to lookup a name from a specific expression's location?

Hi, I’m writing a check that detects and removes redundant this-> usage. Finding explicit this-> was simple, now I want to check whether it’s safe to remove it. My main concern is such case:

I /believe/ that’s not entirely possible in clang (DeclContexts are not preserved) - though I guess clangd does /something/ to try to get good-enough name lookup from arbitrary locations (maybe it reparses the code to achieve this? Not sure)

Hi, I'm writing a check that detects and removes redundant `this->` usage. Finding explicit `this->` was simple, now I want to check whether it's safe to remove it. My main concern is such case:
struct X{
     void f(int x){
         // without `this->` will use parameter

         // can remove `this->` because
         // local `y` is not visible yet

         int y;
     int x;
     int y;
I need to know whether a specific name(function or variable) will resolve to the same declaration without `this->` part. My matcher is /memberExpr(has(cxxThisExpr()))/, I bind MemberExpr and CXXThisExpr, I can rewrite matcher to also bind enclosing CXXMethodDecl. As I understand, simple enumeration of parameter/local variable names won't work(as in case with `y`) because of lookup/visibility rules. Is there some `lookup()` function whose result I can compare against the declaration of original function/variable? I found Sema::LookupName() but can't figure out how to get Scope of the found expression. Can you give me an example please?

The Scope is a parser thing which contains an opaque ptr to the DeclContext. In principle, you can create a fake Scope and attach the relevant DeclContext via setEntity. I think just passing Sema::TUScope may work. If you want some pseudocode:

Scope *fakeS = new Scope(...);
// you may need to do some work for the variable shadowing
LookupResult R(Sema, Var->getName(), SourceLocation(), Sema::LookupOrdinaryName, Sema::ForRedeclaration);
Sema->LookupName(R, fakeS);
delete fakeS;

Note that is an approximation as we do not attach the scope to the chain of scopes like the parser does. It some extra work that is also possible.

Removing this-> from this->y is safe in the sense that the code will still work, because the local y isn’t visible yet, but it would be very confusing and error prone for the human reading it. It would subtly break if someone moves the local y declaration higher for example.

So maybe you should consider not suggesting the removal of this-> in a situation like this (when there is a local var with the same name but it’s not yet visible) due to code readability, even if it’s technically safe to remove it.

And as a bonus, it becomes easier to implement :slight_smile:

A general thought:

What makes this perhaps unnecessarily difficult is that ASTConsumer does not provide a full suite of overrideable Handle* functions to be called in the midst of parsing.

There is HandleTranslationUnit() (called after the full translation unit has been parsed), HandleTopLevelDecl() (called after each top-level decl is parsed), and the remainder seem to be assorted special cases (HandleInterestingDecl() etc).

If there were e.g. HandleCXXThisExpr(CXXThisExpr *E) et al, called while E’s proper scope/state data is still active, Oleksandr could define his logic in an override of that, within which he could simply call S.LookupName(Result, S.getCurScope()) to get his lookup results. No need to copy and paste implementation details to reenter the proper scope/state.

Re how it could be implemented to minimize overhead:

// — ASTConsumer.h — //
class AbstractDeclHandler {…}; // Like DeclVisitor but using virtuals instead of CRTP
class AbstractStmtHandler {…}; //""
class AbstractExprHandler {
… //e.g.:
virtual bool HandleCXXThisExpr(Expr *E) { return true; }
class ASTConsumer {
AbstractDeclHandler *DH = nullptr;
AbstractStmtHandler *SH = nullptr;
AbstractExprHandler *EH = nullptr;
//getDeclHandler/setDeclHandler/et al

// - lib/Sema/… - //
ExprResult Sema::BuildCXXThisExpr(…) {
ExprResult res = …;

if (auto *Handler = Consumer.getExprHandler())
if (!Handler->HandleCXXThisExpr(res.get()))
return ExprError();

return res;
// same for other Stmts & Decls: always Handle* after building

To further reduce overhead: “handling” Exprs immediately upon building is too fine-grained — probably only need to handle Stmts and Decls immediately after building them, and group the Expr handling calls together in the handling of their parent Stmt.

But this at least demonstrates how a user override called in this way would have access to all the proper scope/state info originally available to the node, which could be a very useful addition to ASTConsumer.