Static Code Analysis for OpenSCAD

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

Static Code Analysis for OpenSCAD

julianstirling
Hi Everyone,

I am part of the OpenFlexure project. An OpenSCAD based 3D printed
laboratory grade microscope (openflexure.org).

As part of trying to tidy up the OpenFlexure Microscope code base I have
started to write a linter/static code analyser for OpenSCAD. The project is
written in Python, you can install it with `pip install sca2d` or clone it
from
https://gitlab.com/bath_open_instrumentation_group/sca2d

SCA2D is only 4 days old so don't expect the world. I have concentrated on
trying to lex and parse the scad properly so I can check for things like
variables being redefined from parent scopes or other such code that can
cause confusing issues. I am yet to write proper unit tests for the code,
but it seems to parse the microscope repository (which is quite complicated)
with no problems. I have defined the OpenSCAD grammar in ENBF format. I did
a bit of a check and couldn't find and ENBF or BFN file for OpenSCAD in the
main repo?

It would be great if others could try it on a range of files and let me know
if it fails to parse valid files or claims invalid files are fine. I would
also be interested to hear the OpenSCAD community's perspectives on good
code style checks to include.

I am planning to follow a pylint ethos where the linting is configurable,
because we all have different needs. I am far from being able to do this yet
as the messaging system for the warnings is pretty ad-hoc. As we are hoping
to move the microscope towards medical certification we will probably opt
for very strict code style which may not be appropriate for all projects.

If you have any thoughts contact me here or open an issue on the SCA2D
GitLab page.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

lar3ry
Sounds great!
Unfortunately, I am pretty much unfamiliar with python, so please bear with
me if I am asking the obvious.
I see a setup.py program. Do I run that to install it on a Windows machine?
If so, will it install the main program and libraries in the appropriate
places?

If so, I may have done something wrong, as I do not see even the sca2d.py
program in any of my python directories. I have three directories:
python37, python37-32, and python38-32

If I have to put the programs/libs in particular places,  where would they
be?
And should I get rid of some of these versions of python?





--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

julianstirling
Hi lar3ry,

Sorry for the confusion. I am not really much of a windows user so I am not
sure how much of a problem the multiple versions will be. I should have
explained how to install it. You can run from the setup.py but it is much
easier to get the releases from PyPi.

Hopefully one of your pythons is on the PATH. If you open up powershell and
type:
    pip install sca2d
Then hopefully it will install sca2d. You should then just be able to run:
    sca2d filename.scad
without having put a py file through python. It does all depend on python
being on the PATH. If you have problems then maybe removing the current
versions of python and starting again from the python website, but make sure
during install that you select the check box to add python to the path.

Hope this helps



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
tp3
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

tp3
In reply to this post by julianstirling
On 20.11.20 01:21, julianstirling wrote:
> I did a bit of a check and couldn't find and ENBF or BFN file
> for OpenSCAD in the main repo?

No separate EBNF, but there's a bison parser definition in
https://github.com/openscad/openscad/blob/master/src/parser.y

ciao,
  Torsten.

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
-- Torsten
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

nophead
I installed it with pip3 and I do have python38-32 in my path but it comes up with this:

Traceback (most recent call last):
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\runpy.py", line 192, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Users\ChrisP\AppData\Local\Programs\Python\Python38-32\Scripts\sca2d.exe\__main__.py", line 7, in <module>
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\__main__.py", line 29, in main
    analyser = Analyser()
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\sca2d.py", line 41, in __init__
    self._parser = ScadParser()
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\sca2d.py", line 17, in __init__
    self._parser = self._create_parser()
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\sca2d.py", line 24, in _create_parser
    with open(scad_lark_filename, 'r') as scad_lark_file:
FileNotFoundError: [Errno 2] No such file or directory: 'c:\\users\\chrisp\\appdata\\local\\programs\\python\\python38-32\\lib\\site-packages\\sca2d\\lark\\scad.lark'

On Fri, 20 Nov 2020 at 12:27, Torsten Paul <[hidden email]> wrote:
On 20.11.20 01:21, julianstirling wrote:
> I did a bit of a check and couldn't find and ENBF or BFN file
> for OpenSCAD in the main repo?

No separate EBNF, but there's a bison parser definition in
https://github.com/openscad/openscad/blob/master/src/parser.y

ciao,
  Torsten.

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

julianstirling
@Nophead Totally my fault, I never tested the wheel, I included the lark file
incorrectly. I have patched it now. Can you try running:
   pip install sca2d --upgrade
and then trying again.

@tp3 - I did have a look at that file but it confused me, I will look into
bison. I will have a think about if I want to jump ship from ENBF to bison.
I quite like the ENBF style. So as long as I keep them equivalent it should
be fine.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

nophead
Yes that installs correctly, thanks.

I ran it on my library test file which includes all of my library but it doesn't like a lot of the syntax and eventually falls over with a Python error.

Traceback (most recent call last):
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\runpy.py", line 192, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Users\ChrisP\AppData\Local\Programs\Python\Python38-32\Scripts\sca2d.exe\__main__.py", line 7, in <module>
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\__main__.py", line 30, in main
    parsed = analyser.analyse_file(args.filename, output_tree=args.output_tree)
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\sca2d.py", line 86, in analyse_file
    scope.analyse_tree(self._parsed_files)
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\outerscope.py", line 112, in analyse_tree
    self._check_defintions(parsed_files)
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\outerscope.py", line 132, in _check_defintions
    [var_defs, mod_defs, func_defs] = inc_scope.get_include_defintions(parsed_files,
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\outerscope.py", line 71, in get_include_defintions
    def_var, def_mod, def_func = inc_scope.get_include_defintions(parsed_files,
  File "c:\users\chrisp\appdata\local\programs\python\python38-32\lib\site-packages\sca2d\outerscope.py", line 62, in get_include_defintions
    if filename in breadcrumbs:
TypeError: argument of type 'NoneType' is not iterable


Valid OpenSCAD syntax it does not like :
each in a list comprehension. It doesn't seem to like list comprehensions in general, or let. They are departures for C like syntax.

`vitamins\screws.scad`:0:0: F0001: Cannot read file due to syntax error:
   - No terminal defined for 'e' at line 122 col 35
   -
   - screws = [for(list = screw_lists) each list];
   -                                   ^
   -
   - Expecting: {'__ANON_2', '__ANON_5', 'MORETHAN', 'QMARK', 'SLASH', '__ANON_1',

   -
If you belive this is a bug in SCA2D please report it to us.

Variables can begin with numbers in OpenSCAD.

parsing vitamins\pin_headers.scad
`vitamins\pin_headers.scad`:0:0: F0001: Cannot read file due to syntax error:
   - No terminal defined for '2' at line 27 col 1
   -
   - 2p54header    = ["2p54header",   2.54, 1
   - ^
   -
   - Expecting: {'INCLUDE', 'FOR', 'IF', 'LET', 'CALL_NAME', 'VARIABLE', 'USE', '__ANON_0', 'LBRACE', 'MODULE', 'TERMINATION', 'FUNCTION'}
   -
If you belive this is a bug in SCA2D please report it to us.

Arrays can be indexed by field names .x, .y, ,.z and some more I can't remember.

`vitamins\led_meter.scad`:0:0: F0001: Cannot read file due to syntax error:
   - No terminal defined for '.' at line 39 col 49
   -
   -  meter_pos(type) = (meter_pcb_size(type).y - meter_size(type).y) * meter_offset(
   -                                         ^
   -
   - Expecting: {'__ANON_2', 'RPAR', '__ANON_5', 'MORETHAN', 'QMARK', 'SLASH', '__ANON_1', '__ANON_4', 'PERCENT', 'STAR', 'LSQB', '__ANON_6', 'MINUS', '__ANON_3', 'LESSTHAN', 'PLUS'}
   -
If you belive this is a bug in SCA2D please report it to us.


Plus many many more syntax errors. You might want to clone NopSCADlib and test it yourself because there are certainly no syntax errors in it because you can load that file in OpenSCAD and it generates the cover picture for the library.



On Fri, 20 Nov 2020 at 13:31, julianstirling <[hidden email]> wrote:
@Nophead Totally my fault, I never tested the wheel, I included the lark file
incorrectly. I have patched it now. Can you try running:
   pip install sca2d --upgrade
and then trying again.

@tp3 - I did have a look at that file but it confused me, I will look into
bison. I will have a think about if I want to jump ship from ENBF to bison.
I quite like the ENBF style. So as long as I keep them equivalent it should
be fine.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

lar3ry
This post was updated on .
In reply to this post by julianstirling
Python37 is in PATH, and .py is in PATHEXT.
I cd to sca2d-master\sca2d and run
pip install sca2d
 I get an error:

Fatal error in launcher: Unable to create process using
'"c:\users\admin\appdata\local\programs\python\python37\python.exe"
"C:\Users\Admin\AppData\Local\Programs\Python\Python37\Scripts\pip.exe"
install sca2d'

But not to worry. I'll wait until someone has successfully installed it on
Windows.

Please disregard this. I was editing this and checked something out. I did not tell it to post, but there it was.

python.exe was not in python37. I put it there, and sca2d.py installed. Thanks.

--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
Discuss@lists.openscad.org
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

julianstirling
@nophead Thanks again.

I had no idea about the .x .y .z indexing, and yes I missed the generators.
I suppose I was biased to the way we write SCAD. I will make sure that I can
get it all running on of NopSCAD. It is a fantastic resource, thank you!

The other error is related to how I am handling imports. I will look into
the problem in my code that caused this error and make sure it is fixed.

@lar3ry Hmm that is a strange message. installing with pip should not need
you to CD anywhere as it will pull the package down from the internet. I
wonder if the problem is with pip on your machine? What happens if you run:
pip install pyyaml
Pyyaml is a pretty well established package, so if that won't work then it
is a python installation issue.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

lar3ry
Please reread my previous, which I have edited.
It should not have posted.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

lar3ry
In reply to this post by julianstirling
Just a few things I have noticed.
I ran it against an OpenSCAD file designed to be run through the customizer,
and saw a few things.

parsing publicDomainGearV1.1.scad
publicDomainGearV1.1.scad:63:2: W2002: Overwriting `mm_per_tooth` variable
from a lower scope.
publicDomainGearV1.1.scad:65:2: W2002: Overwriting `thickness` variable from
a lower scope.
publicDomainGearV1.1.scad:74:47: W2008: Variable `pi` used but never
defined.
publicDomainGearV1.1.scad:75:14: W2008: Variable `p` used but never defined.
...
publicDomainGearV1.1.scad:79:9: W1001: Overly complicated argument contains
9 tokens.

I wrote a small test program to check a few of these, and got:

parsing Test.scad
Test.scad:2:15: W2002: Overwriting `x` variable from a lower scope.

Test1 illustrates the first problem. In a module, a parameter can have a
default value.
Test2 does not duplicate the errors shown for lines 74 and 75 of
publicDomainGearV1.1.scad

About the line 79:9 "Overly complicated argument", I'm not sure that's a
problem. I admit I am not an expert OpenSCAD user, but it seems to me that a
valid startement using any number of arguments should not generate a warning
or error.

publicDomainGearV1.scad
<http://forum.openscad.org/file/t2121/publicDomainGearV1.scad>  
Test.scad <http://forum.openscad.org/file/t2121/Test.scad>  

Nice work.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

julianstirling
Yeah exactly what rules we put in are an interesting question and there will
always be differences of opinion, and the best way to deal with this is to
make them customisable.


For context of the microscope we are looking to move the microscope design
towards something that could be medically certified. For this we need a lot
of confidence that every piece of code behaves as expected.

Overwriting a variable from a lower scope can cause a piece of code to
change behaviour unexpectedly or to confuse someone on which variable is
being used. It is then safer to pass in arguments than overwrite a variable.
We are also likely to warn that include in itself is dangerous. So in the
example of `Test.scad` if you remove line 1 then it doesn't warn. So it is
fine to override the the default input, but it warns if a variable in that
scope has the same name as one defined in the lower scope. These warnings
are annoying to a lot of people. So we may perhaps turn them off by default
and have a mode that is more like
sca2d --strict file.scad

The same logic goes for overly complicated argument. It is again for
readability and understandability of out code. The idea is to try get
someone to change:

    cube([17*2+0.1*25.4*2+35, 17*2+0.1*25.4*2+75, 17*2+0.1*25.4*2+45]);

To:

    wall_thickness = 17;
    inch = 25.4;
    //clearance is 1/10 of an inch as recommended by the manufacturer on p19
    clearance = 0.1*inch;
    inner_dimensions = [35, 75, 45];
    extra_size = (wall_thickness+clearance)*2*[1,1,1];
    cube(inner_dimensions + extra_size);

Both should be identical. But one is more understandable. Of course, the
analyser can only go so far. There are always ways just define a pointless
badly named variable just to reduce the number of tokens. But the analyser
is to help you identify confusing code. Most statements the analyser picks
up will be valid code, they just are things that may confuse someone else
reading it.

The key to this has to be configurability so a project can decide what the
want to allow. It would be interesting to get feedback from the community
about what dangerous/confusing code they would want to be warned about.



--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

adrianv
julianstirling wrote

> Yeah exactly what rules we put in are an interesting question and there
> will
> always be differences of opinion, and the best way to deal with this is to
> make them customisable.
>
>
> For context of the microscope we are looking to move the microscope design
> towards something that could be medically certified. For this we need a
> lot
> of confidence that every piece of code behaves as expected.
>
> Overwriting a variable from a lower scope can cause a piece of code to
> change behaviour unexpectedly or to confuse someone on which variable is
> being used.

I haven't tried the analyzer, but I think that in a case like this:

function myfunc(parm1, parm2) =
  let(
       parm2 = is_undef(parm2) ? parm1 : parm2,
      ....
  )

it is more clear to define parm2 in the lower scope as I do above than it
would be to do something like

function myfunction(parm1, parm2_temp)
  let(
      parm2 = is_undef(parm2_temp) ? parm1: parm2_temp,
     ....
  )


>  It is then safer to pass in arguments than overwrite a variable.

Technically speaking it's impossible to overwrite a "variable" in OpenSCAD.
You can only define new ones.  




--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

OpenSCAD on Apple Silicon

Whosawhatsis
Has anyone tried OpenSCAD on any of the new Apple Silicon machines (MacBook Air/Pro or Mac Mini with the M1 processor)?

The latest nightly doesn't appear to be compiled as Universal 2. Although Rosetta 2 seems to be handling most things really well by all reports, I can easily see the kind of stuff that OpenCSG is doing potentially having bugs on the new GPUs.

I'm looking to upgrade soon, but I really need to be able to run OpenSCAD, and its support for the new machines is currently my biggest question mark.

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
tp3
Reply | Threaded
Open this post in threaded view
|

Re: OpenSCAD on Apple Silicon

tp3
On 21.11.20 01:14, Whosawhatsis wrote:
> Has anyone tried OpenSCAD on any of the new Apple Silicon machines
> (MacBook Air/Pro or Mac Mini with the M1 processor)?

Yep, that would be interesting to hear.

> The latest nightly doesn't appear to be compiled as Universal 2.
> Although Rosetta 2 seems to be handling most things really well
> by all reports, I can easily see the kind of stuff that OpenCSG
> is doing potentially having bugs on the new GPUs.

I don't know if it will be possible to do anytime soon. Right
now it's not clear how to support MacOS if more of the services
currently free for open source projects are closing down and/or
are limiting availability.

ciao,
  Torsten.

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
-- Torsten
Reply | Threaded
Open this post in threaded view
|

Re: OpenSCAD on Apple Silicon

doug.moen
Looks like Github Actions has a MacOS 11 build environment.
https://github.com/actions/virtual-environments/issues/1814
It is free for public repositories.

On Fri, Nov 20, 2020, at 7:42 PM, Torsten Paul wrote:

> On 21.11.20 01:14, Whosawhatsis wrote:
> > Has anyone tried OpenSCAD on any of the new Apple Silicon machines
> > (MacBook Air/Pro or Mac Mini with the M1 processor)?
>
> Yep, that would be interesting to hear.
>
> > The latest nightly doesn't appear to be compiled as Universal 2.
> > Although Rosetta 2 seems to be handling most things really well
> > by all reports, I can easily see the kind of stuff that OpenCSG
> > is doing potentially having bugs on the new GPUs.
>
> I don't know if it will be possible to do anytime soon. Right
> now it's not clear how to support MacOS if more of the services
> currently free for open source projects are closing down and/or
> are limiting availability.
>
> ciao,
>   Torsten.
>
> _______________________________________________
> OpenSCAD mailing list
> [hidden email]
> http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
>

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

lar3ry
In reply to this post by julianstirling
julianstirling wrote
> Yeah exactly what rules we put in are an interesting question and there
> will
> always be differences of opinion, and the best way to deal with this is to
> make them customisable.

Agreed.


> Overwriting a variable from a lower scope can cause a piece of code to
> change behaviour unexpectedly or to confuse someone on which variable is
> being used. It is then safer to pass in arguments than overwrite a
> variable.
> We are also likely to warn that include in itself is dangerous. So in the
> example of `Test.scad` if you remove line 1 then it doesn't warn. So it is
> fine to override the the default input, but it warns if a variable in that
> scope has the same name as one defined in the lower scope. These warnings
> are annoying to a lot of people. So we may perhaps turn them off by
> default
> and have a mode that is more like
> sca2d --strict file.scad

Sounds good. I see a LOT of people using defaults for variables, and using
the same names. I can understand the reasoning on both sides. Some want to
name it the same as the external one so it is clear what its purpose is, and
to supply a default value. Others (possibly those that see variables in
openSCAD as being actual variables), as being redefined. Both sides might
see the opposite as confusing.

You did not mention it, but what about the "variable used but never
defined"?

publicDomainGearV1.1.scad:74:47: W2008: Variable `pi` used but never
defined.
publicDomainGearV1.1.scad:75:14: W2008: Variable `p` used but never defined.

I see that when I remove the assign() (which is deprecated), it no longer
gets flagged, so that may be the reason for it.


> The same logic goes for overly complicated argument. It is again for
> readability and understandability of out code. The idea is to try get
> someone to change:
>
>     cube([17*2+0.1*25.4*2+35, 17*2+0.1*25.4*2+75, 17*2+0.1*25.4*2+45]);
>
> To:
>
>     wall_thickness = 17;
>     inch = 25.4;
>     //clearance is 1/10 of an inch as recommended by the manufacturer on
> p19
>     clearance = 0.1*inch;
>     inner_dimensions = [35, 75, 45];
>     extra_size = (wall_thickness+clearance)*2*[1,1,1];
>     cube(inner_dimensions + extra_size);

Fair enough, but there are things that simply can't be reduced, so where
would you set the limit?
In the file I referenced, line 123:7 is a good case in point. All those
variables are used in the module.


> Both should be identical. But one is more understandable. Of course, the
> analyser can only go so far. There are always ways just define a pointless
> badly named variable just to reduce the number of tokens. But the analyser
> is to help you identify confusing code. Most statements the analyser picks
> up will be valid code, they just are things that may confuse someone else
> reading it.
>
> The key to this has to be configurability so a project can decide what the
> want to allow. It would be interesting to get feedback from the community
> about what dangerous/confusing code they would want to be warned about.

Well, this has been feedback for you, and I am happy that you see the
desirability of allowing preferences to be set.

I do like the "Function defined but never used" and the "Semicolon not
required" reporting.

Again, nice work.




--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

julianstirling
@lar3ry
"You did not mention it, but what about the "variable used but never
defined"? "

So used but never defined, should catch an error, that will happen in some
paths of the code. I can well believe there is a bug in how I count it at
the moment. The program is less than a week old :)

The "X defined but never used" warnings need a bit of nuance. A library of
modules/functions that are for others to "use", or a library of variables
for others to "include". They are defined but not used for a very good
reason! At the moment SCA2D just shouts about all of them. I think that
defining something in a module/control scope and never using should generate
a warning. In the outer-scope, it probably should not for libraries.

I say "it should not for libraries" but obviously any file can be used as a
library. I think this is where again we may for our specific purposes have a
strict mode where we somehow divide files into libraries that define modules
and functions but don't use them. And geometry files that use everything
they declare. This is a useful separation for our purposes but not a sane
default I would want to "impose" on others.

"Fair enough, but there are things that simply can't be reduced, so where
would you set the limit?" This I think is the key question. One thing I am
starting to do now is recategorise the warnings more sensibly. Using
something before it is defined is an Error and should have an E code, not
using something within a module scope is a Warning and should have a W code.
Complexity should be listed under "consider refactoring" with an R code.

I think the important thing about code analysis is that there is always the
option to ignore the message. So when I program PyLint tells me off when a
class has too many attributes, or when a method is too complex. Sometime I
will look at the Class/Method and go "Eh, you know, it is fine as it is.".
Other times I will look at it and go "Wow that is getting out of hand, I
should tidy it up.". The code analysis is a great way to locate potentially
confusing things and then deciding if you want to fix them. However, if you
think it is fine, then the right thing to do is ignore the analysis. In
PyLint I used to put in little codes to disable a specific warning in a
specific place. I have stopped doing this now because as the code mature
these things grow until I really should look at them again.

Looking now at your specific example it is a long array... And a very clear
long array. I think I do really need a better way to calculate the
complexity of arrays because clearly the expression you flagged is a very
very clearly laid out expression. I have made a note of this and will fix
it. It won't be a quick fix because my complexity calculation is very overly
simplistic right now. Thanks for flagging this!


@adrianv

That example with the "let" is very interesting. I had not appreciated that
let could be used in that way. In fact, none of our code uses let in any way
so I just bashed it into the lexer/parser without much further thought.
Reading in more detail I do actually need to give it significantly more
thought as it is a very powerful new feature. Thanks for bringing my
attention to this :)





--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

acwest
I'm not sure how useful complexity warnings are in OpenScad. The nature of the language requires some very complex syntax to accomplish almost any non-trivial task... 

On Sat, 21 Nov 2020, 07:37 julianstirling, <[hidden email]> wrote:
@lar3ry
"You did not mention it, but what about the "variable used but never
defined"? "

So used but never defined, should catch an error, that will happen in some
paths of the code. I can well believe there is a bug in how I count it at
the moment. The program is less than a week old :)

The "X defined but never used" warnings need a bit of nuance. A library of
modules/functions that are for others to "use", or a library of variables
for others to "include". They are defined but not used for a very good
reason! At the moment SCA2D just shouts about all of them. I think that
defining something in a module/control scope and never using should generate
a warning. In the outer-scope, it probably should not for libraries.

I say "it should not for libraries" but obviously any file can be used as a
library. I think this is where again we may for our specific purposes have a
strict mode where we somehow divide files into libraries that define modules
and functions but don't use them. And geometry files that use everything
they declare. This is a useful separation for our purposes but not a sane
default I would want to "impose" on others.

"Fair enough, but there are things that simply can't be reduced, so where
would you set the limit?" This I think is the key question. One thing I am
starting to do now is recategorise the warnings more sensibly. Using
something before it is defined is an Error and should have an E code, not
using something within a module scope is a Warning and should have a W code.
Complexity should be listed under "consider refactoring" with an R code.

I think the important thing about code analysis is that there is always the
option to ignore the message. So when I program PyLint tells me off when a
class has too many attributes, or when a method is too complex. Sometime I
will look at the Class/Method and go "Eh, you know, it is fine as it is.".
Other times I will look at it and go "Wow that is getting out of hand, I
should tidy it up.". The code analysis is a great way to locate potentially
confusing things and then deciding if you want to fix them. However, if you
think it is fine, then the right thing to do is ignore the analysis. In
PyLint I used to put in little codes to disable a specific warning in a
specific place. I have stopped doing this now because as the code mature
these things grow until I really should look at them again.

Looking now at your specific example it is a long array... And a very clear
long array. I think I do really need a better way to calculate the
complexity of arrays because clearly the expression you flagged is a very
very clearly laid out expression. I have made a note of this and will fix
it. It won't be a quick fix because my complexity calculation is very overly
simplistic right now. Thanks for flagging this!


@adrianv

That example with the "let" is very interesting. I had not appreciated that
let could be used in that way. In fact, none of our code uses let in any way
so I just bashed it into the lexer/parser without much further thought.
Reading in more detail I do actually need to give it significantly more
thought as it is a very powerful new feature. Thanks for bringing my
attention to this :)





--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
Reply | Threaded
Open this post in threaded view
|

Re: Static Code Analysis for OpenSCAD

julianstirling
I have pushed out v0.0.4. You can get the newest version by running:
    pip install sca2d --upgrade

Thanks to the the helpful feedback of @lar3ry I have realised I was handling
assign statements incorrectly. This is fixed so they throw a sensible
depreciation message not a variable used before definition error.

Also thanks to @adrianv, I have now implemented let expressions in the form
he mentioned. It does still throw the variable overwritten from lower scope
warning. I need to think about this some more.

I have also recategorised the warnings into Errors, Warnings, Depreciation
warnings, and Information. These have E, W, D and I codes. If you run:
    sca2 --colour file.scad
it will colourise the errors (not checked this on Windows!). The Fatal
warnings when files cannot be read still sit outside my normal messaging
system so they are still just standard in colour.

I still haven't managed to get to some of the other errors that @nophead
found. But I have added the problems to the README.

Finally to @acwest. I think that the nature of OpenSCAD is that it is a
low-level language. While you need to do quite a number of commands to
accomplish tasks, there is nothing stopping you breaking these up into
variable, smaller functions and modules. The concepts of cognitive
complexity and cyclotomic complexity have been for programming date back to
the 1980s or before, back when almost all languages were low level and
required a large number of commands to perform a simple task. The idea is to
break things down into managable chunks that can be digested.




--
Sent from: http://forum.openscad.org/

_______________________________________________
OpenSCAD mailing list
[hidden email]
http://lists.openscad.org/mailman/listinfo/discuss_lists.openscad.org
12