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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Manas via cfe-dev
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
        this->x++;

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

        int y;
        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?

--
Regards,
Oleksandr Koval.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Manas via cfe-dev
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)

On Sat, Oct 3, 2020 at 6:06 PM Oleksandr Koval via cfe-dev <[hidden email]> wrote:
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
        this->x++;

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

        int y;
        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?

--
Regards,
Oleksandr Koval.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Manas via cfe-dev
In reply to this post by Manas via cfe-dev
On 10/3/20 3:40 PM, Oleksandr Koval via cfe-dev wrote:
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
        this->x++;

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

        int y;
        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(...);
  fakeS->setEntity(DC);
  // 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.



--
Regards,
Oleksandr Koval.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Manas via cfe-dev
In reply to this post by Manas via cfe-dev

El 3 oct. 2020, a la(s) 22:06, Oleksandr Koval via cfe-dev <[hidden email]> escribió:


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
        this->x++;

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

        int y;
        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?

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 :)

-- 
Nicolás

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Manas via cfe-dev
In reply to this post by Manas via cfe-dev
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;
public:
  //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.

On Oct 4, 2020, at 3:15 AM, Vassil Vassilev via cfe-dev <[hidden email]> wrote:

On 10/3/20 3:40 PM, Oleksandr Koval via cfe-dev wrote:
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
        this->x++;

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

        int y;
        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(...);
  fakeS->setEntity(DC);
  // 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.



--
Regards,
Oleksandr Koval.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev