semgrep / pfff

pfff is mainly an OCaml API to write static analysis, dynamic analysis, code visualizations, code navigations, or style-preserving source-to-source transformations such as refactorings on source code.
https://semgrep.dev
Other
186 stars 29 forks source link

Made self and parent their own grammar nodes #516

Closed wu-s-john closed 2 years ago

wu-s-john commented 2 years ago

This is a PR that helps solves the issue, PHP, not recognizing const variables as constant strings

The corresponding semgrep changes are made here. https://github.com/returntocorp/semgrep/pull/5022

Security

IagoAbal commented 2 years ago

I don't know much about PHP (maybe @nmote know some PHP?), but overall it looks good to me! Could you take a look at my comments? Thanks for contributing!

wu-s-john commented 2 years ago

@aryx

Here is the test plan and the parsing stats affected by my PR.

In the end, the changes to the parsing code was very slight. You can see the changes below where the expression self is translated to IdSpecial((Self, ())

Test Plan

$ cat  semgrep-core/tests/php/constant_propogation.php
<?php

class Example
{
    const CMD = 'This is an constant.';

    public function someMethod()
    {
        // ERROR:
        exec(self::CMD);
    }
}
$ pfff -dump_php semgrep-core/tests/php/constant_propogation.php #cst_php
[(ClassDef
    { c_attrs = None; c_type = (ClassRegular ());
      c_name = (Name ("Example", ())); c_tparams = None; c_extends = None;
      c_implements = None; c_enum_type = None;
      c_body =
      ((),
       [(ClassConstants ([], (), None,
           [(Left
               ((Name ("CMD", ())),
                ((), (Sc (C (String ("This is an constant.", ())))))))
             ],
           ()));
         (Method
            { f_attrs = None; f_tok = (); f_type = MethodRegular;
              f_modifiers = [(Public, ())]; f_ref = None;
              f_name = (Name ("someMethod", ())); f_tparams = None;
              f_params = ((), [], ()); f_return_type = None;
              f_body =
              ((),
               [(ExprStmt (
                   (Call ((Id (XName [(QI (Name ("exec", ())))])),
                      ((),
                       [(Left
                           (Arg
                              (ClassGet ((Id (Self ())), (),
                                 (Id (XName [(QI (Name ("CMD", ())))]))))))
                         ],
                       ())
                      )),
                   ()))
                 ],
               ())
              })
         ],
       ())
      });
  (FinalDef ())]
$ semgrep-core -dump_pfff_ast -lang php semgrep-core/tests/php/constant_propogation.php #ast_php
Pr(
  [DefStmt(
     ({
       name=EN(
              Id(("Example", ()),
                {id_info_id=5; id_hidden=false; id_resolved=Ref(None); 
                 id_type=Ref(None); id_svalue=Ref(None); }));
       attrs=[]; tparams=[]; },
      ClassDef(
        {ckind=(Class, ()); cextends=[]; cimplements=[]; cmixins=[]; 
         cparams=[]; 
         cbody=[F(
                  DefStmt(
                    ({
                      name=EN(
                             Id(("CMD", ()),
                               {id_info_id=1; id_hidden=false; 
                                id_resolved=Ref(None); id_type=Ref(None); 
                                id_svalue=Ref(None); }));
                      attrs=[KeywordAttr((Const, ()))]; tparams=[]; },
                     VarDef(
                       {vinit=Some(L(String(("This is an constant.", ())))); 
                        vtype=None; }))));
                F(
                  DefStmt(
                    ({
                      name=EN(
                             Id(("someMethod", ()),
                               {id_info_id=4; id_hidden=false; 
                                id_resolved=Ref(None); id_type=Ref(None); 
                                id_svalue=Ref(None); }));
                      attrs=[KeywordAttr((Public, ()))]; tparams=[]; },
                     FuncDef(
                       {fkind=(Method, ()); fparams=[]; frettype=None; 
                        fbody=FBStmt(
                                Block(
                                  [ExprStmt(
                                     Call(
                                       N(
                                         Id(("exec", ()),
                                           {id_info_id=2; id_hidden=false; 
                                            id_resolved=Ref(None); 
                                            id_type=Ref(None); 
                                            id_svalue=Ref(None); })),
                                       [Arg(
                                          DotAccess(IdSpecial((Self, ())),
                                            (),
                                            FN(
                                              Id(("CMD", ()),
                                                {id_info_id=3; 
                                                 id_hidden=false; 
                                                 id_resolved=Ref(None); 
                                                 id_type=Ref(None); 
                                                 id_svalue=Ref(None); }))))]),
                                     ())]));
                        }))))];
         })))])

Stats

stats before PR change:

lang/php/stats.json:3
   "global": {
     "name": "*",
     "parsing_rate": 0.8984308332917063,
     "line_count": 5305045,
     "error_line_count": 538829,
     "file_count": 35023,
     "error_file_count": 1498
   },

stats after PR change:

lang/php/stats.json:3
   "global": {
     "name": "*",
     "parsing_rate": 0.8984308332917063,
     "line_count": 5305045,
     "error_line_count": 538829,
     "file_count": 35023,
     "error_file_count": 1498
   },

Overall, doesn't look like anything change

wu-s-john commented 2 years ago

@IagoAbal @aryx

Thanks for the suggestions. I incorporated your feedback. Please take a look again when you have time.

aryx commented 2 years ago

not sure what's going on with circleci not running

wu-s-john commented 2 years ago

@aryx

Thanks for merging the PR.