[v2,1/2] codeparser/utils: clean up more deprecated AST usages

From: Chris Laplante <chris.laplante@agilent.com>

Also introduces bb.utils.ast_node_str_value method to simplify handling
of AST str nodes.

Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
---
 lib/bb/codeparser.py | 22 ++++++++++------------
 lib/bb/utils.py      | 10 ++++++++++
 2 files changed, 20 insertions(+), 12 deletions(-)

On Mon, 2023-10-23 at 12:46 -0400, Chris Laplante via
lists.openembedded.org wrote:
> From: Chris Laplante <chris.laplante@agilent.com>
> 
> Also introduces bb.utils.ast_node_str_value method to simplify handling
> of AST str nodes.
> 
> Signed-off-by: Chris Laplante <chris.laplante@agilent.com>
> ---
>  lib/bb/codeparser.py | 22 ++++++++++------------
>  lib/bb/utils.py      | 10 ++++++++++
>  2 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/bb/codeparser.py b/lib/bb/codeparser.py
> index cd39409434..92ed31fa5f 100644
> --- a/lib/bb/codeparser.py
> +++ b/lib/bb/codeparser.py
> @@ -256,19 +256,18 @@ class PythonParser():
>      def visit_Call(self, node):
>          name = self.called_node_name(node.func)
>          if name and (name.endswith(self.getvars) or name.endswith(self.getvarflags) or name in self.containsfuncs or name in self.containsanyfuncs):
> -            if isinstance(node.args[0], ast.Constant) and isinstance(node.args[0].value, str):
> -                varname = node.args[0].value
> -                if name in self.containsfuncs and isinstance(node.args[1], ast.Str):
> +            if (varname := bb.utils.ast_node_str_value(node.args[0])) is not None:
> +                if name in self.containsfuncs and bb.utils.ast_node_str_value(node.args[1]) is not None:

Personally, I really don't like condition tests modifying variables as
it can be easy to miss when reading the code. I therefore have a bit of
a preference for not doing that.

Perhaps it just takes time to get used to the new syntax, perhaps I'm
just old fashioned.


>                      if varname not in self.contains:
>                          self.contains[varname] = set()
> -                    self.contains[varname].add(node.args[1].s)
> -                elif name in self.containsanyfuncs and isinstance(node.args[1], ast.Str):
> +                    self.contains[varname].add(node.args[1].value)
> +                elif name in self.containsanyfuncs and bb.utils.ast_node_str_value(node.args[1]) is not None:
>                      if varname not in self.contains:
>                          self.contains[varname] = set()
> -                    self.contains[varname].update(node.args[1].s.split())
> +                    self.contains[varname].update(node.args[1].value.split())
>                  elif name.endswith(self.getvarflags):
> -                    if isinstance(node.args[1], ast.Str):
> -                        self.references.add('%s[%s]' % (varname, node.args[1].s))
> +                    if bb.utils.ast_node_str_value(node.args[1]) is not None:
> +                        self.references.add('%s[%s]' % (varname, node.args[1].value))
>                      else:
>                          self.warn(node.func, node.args[1])
>                  else:
> @@ -276,8 +275,7 @@ class PythonParser():
>              else:
>                  self.warn(node.func, node.args[0])
>          elif name and name.endswith(".expand"):
> -            if isinstance(node.args[0], ast.Str):
> -                value = node.args[0].s
> +            if (value := bb.utils.ast_node_str_value(node.args[0])) is not None:
>                  d = bb.data.init()
>                  parser = d.expandWithRefs(value, self.name)
>                  self.references |= parser.references
> @@ -287,8 +285,8 @@ class PythonParser():
>                          self.contains[varname] = set()
>                      self.contains[varname] |= parser.contains[varname]
>          elif name in self.execfuncs:
> -            if isinstance(node.args[0], ast.Str):
> -                self.var_execs.add(node.args[0].s)
> +            if bb.utils.ast_node_str_value(node.args[0]) is not None:
> +                self.var_execs.add(node.args[0].value)
>              else:
>                  self.warn(node.func, node.args[0])
>          elif name and isinstance(node.func, (ast.Name, ast.Attribute)):
> diff --git a/lib/bb/utils.py b/lib/bb/utils.py
> index b401fa5ec7..0ac304163c 100644
> --- a/lib/bb/utils.py
> +++ b/lib/bb/utils.py
> @@ -11,6 +11,7 @@ import re, fcntl, os, string, stat, shutil, time
>  import sys
>  import errno
>  import logging
> +import ast
>  import bb
>  import bb.msg
>  import locale
> @@ -33,6 +34,7 @@ import random
>  import socket
>  import struct
>  import tempfile
> +from typing import Optional
>  from subprocess import getstatusoutput
>  from contextlib import contextmanager
>  from ctypes import cdll
> @@ -1863,3 +1865,11 @@ def lock_timeout(lock):
>          yield held
>      finally:
>          lock.release()
> +
> +def ast_node_str_value(node: ast.AST) -> Optional[str]:
> +    """
> +    Returns node value if it is an `ast.Constant` str; None otherwise
> +    """
> +    if isinstance(node, ast.Constant) and isinstance(node.value, str):
> +        return node.value
> +    return None

We were previously asked about adding typing and again I'm a little
unsure we want to do that. Personally, I think it looks horribly ugly
and I'm not convinced it buys us that much. I know others have other
opinions.

Whilst I am in theory bitbake's maintainer and in theory I can say no,
I know I'll get asked when we're converting everything time and time
again so again I'm really torn on this :/.

Cheers,

Richard

Hi Richard,

> >      def visit_Call(self, node):
> >          name = self.called_node_name(node.func)
> >          if name and (name.endswith(self.getvars) or
> name.endswith(self.getvarflags) or name in self.containsfuncs or name in
> self.containsanyfuncs):
> > -            if isinstance(node.args[0], ast.Constant) and
> isinstance(node.args[0].value, str):
> > -                varname = node.args[0].value
> > -                if name in self.containsfuncs and isinstance(node.args[1], ast.Str):
> > +            if (varname := bb.utils.ast_node_str_value(node.args[0])) is not
> None:
> > +                if name in self.containsfuncs and
> bb.utils.ast_node_str_value(node.args[1]) is not None:
> 
> Personally, I really don't like condition tests modifying variables as it can be
> easy to miss when reading the code. I therefore have a bit of a preference for
> not doing that.
> 
> Perhaps it just takes time to get used to the new syntax, perhaps I'm just old
> fashioned.

I'm happy to submit a v3 without the walrus operator if you prefer :).


> > +
> > +def ast_node_str_value(node: ast.AST) -> Optional[str]:
> > +    """
> > +    Returns node value if it is an `ast.Constant` str; None otherwise
> > +    """
> > +    if isinstance(node, ast.Constant) and isinstance(node.value, str):
> > +        return node.value
> > +    return None
> 
> We were previously asked about adding typing and again I'm a little unsure we
> want to do that. Personally, I think it looks horribly ugly and I'm not convinced it
> buys us that much. I know others have other opinions.
> 
> Whilst I am in theory bitbake's maintainer and in theory I can say no, I know I'll
> get asked when we're converting everything time and time again so again I'm
> really torn on this :/.

IMHO BitBake is so large and complicated that it would benefit from it. I'm a bit biased, being a heavy user of statically-typed languages and an IDE (IntelliJ) that moans loudly about perceived type mismatches. So as with the walrus, I can resubmit if the answer is 'no' :).


Thanks,
Chris