Add a linter mode to SCons to make it harder to make mistakes.

[bdbaddog]

This is a WIP issue to gather all the ideas of what can reasonable be checked when setting, appending, replacing and Environment() variable.

@mwichmann

Started as dicussion on discord

bdbaddogToday at 10:02 AM
I had an idea which probably belongs in #loony-channel ... It should be possible to write a SConstruct Lint mode..
so if you do thinks like env['LIBS']='a' instead of env['LIBS']=['a'], it would warn you etc..
The things an experienced user has done wrong and fixed.
mwichmannToday at 10:05 AM
probably
bdbaddogToday at 10:05 AM
I'm going to file an issue for that so it doesn't get lost..  and we can add ideas to it to flesh out what could be done?
mwichmannToday at 10:07 AM
any kind of checker/linter would be great, been on my wishlist
loonycyborgToday at 10:19 AM
scons scripts are python. Maybe it could be plugin of existing python checker?
bdbaddogToday at 10:22 AM
@loonycyborg - nope..  This would have to hook in to the logic which implements env['XYZ'] = value
mwichmannToday at 10:27 AM
I don't know that that particular one is a problem... env is much like a UserDict, where there's a backing data element (_dict), and pylint doesn't have any trouble with userdicts
but there's other goodies
(or _data, or whatever it's called)
mwichmannToday at 10:39 AM
Yet Another Nitpicky Question:  if a consvar is set to a tuple, should you be able to append to it?
env['FOO'] = ('one', 'two')
env.Append(FOO="three")
what should happen?
bdbaddogToday at 10:43 AM
UserError.. ?
mwichmannToday at 10:45 AM
the current error message is unenlightening and indicates another unexpected path through a rather complex algorithm... AttributeError: 'str' object has no attribute 'insert':
the algorithm here takes new and inserts old in front of it, thus...
bdbaddogToday at 10:46 AM
that's not very helpful error message for sure.
mwichmannToday at 10:46 AM
it does that when they types differ and other conditions are not met, but it guessed wrong here :slight_smile:
bdbaddogToday at 10:50 AM
gotcha
loonycyborgToday at 11:01 AM
would be cool if env variables had some inherent type. Like if variable type is known as list then Appending either list or single item always has well defined semantic
bdbaddogToday at 11:02 AM
Moving to #loony-channel

Followed by

bdbaddogToday at 11:02 AM
@loonycyborg - yes that would make sense for some variables..
All of which (I think) falls under the umbrella of "make it harder to do bad/silly/un-sconsian things"
or make it easier to figure out how to do the "best" thing?
loonycyborgToday at 11:03 AM
Just to make things less tricky
if Append(x = "x") and Append (x = ["x"]) always does the same thing then it becomes less error prone
actually this can be formulated not as strict typing but rather having a strict "kind"
where "kind" is either scalar or list or mapping.
probably it's a good idea to make some env variables to always have some particular "kind"
and scons adjusting all operations like Append to act according to it
so far I encountered many issues due to particular variable having unexpected kind
while issue of wrong type is uncommon since everything tends to be string, or something stringifiable

[loonycyborg]

Each variable can be one of following: unset, None, scalar, list, mapping. Also value that is passed to Append() etc can be anything of the above. That creates quite a combinatorial explosion. I think it's impossible to always do the right thing without knowing what is the variable expected to be. Like LIBS is supposed to be a list, SHLIBPREFIX is supposed to be a scalar etc. Currently SCons sets initial values to some variables that make things less unpredictable but still if you assign something directly to env["something"] you just overwrite it. I think safest would be to have some auto-coercion feature but it would need to hook directly into operations like []/__getitem__()/__setitem__() and also there would have to be a way to tag a variable as particular kind, i.e. whether it's scalar or list or mapping.

[mwichmann]

I've gotten deeper into this than I ever wanted to so plenty of thoughts here, will probably dribble them out over time.

There is some auto-coercion - e.g. if you Append to CPPDEFINES, it immediately forces the existing value into a list. Well, if it was a string. LIBS needs to do the same bue doesn't. The special-casing extends only to Append and AppendUnique (with the latter containing some logic errors), and misses Prepend and PrependUnique. The idea of having four separate implementations of almost-the-same-thing was bound to lead to unintented divergence, and we have it. See #3876 for more on that.

[loonycyborg]

I managed to implement all combinations of {Append|Prepend}{Non-unique|Unique} in a single C++ template function: https://github.com/loonycyborg/scons-plusplus/blob/master/src/python_interface/environment_wrappers.hpp#L165 But direct assignment is still not covered, so it's possible to unexpectedly assign a string to env["LIBS"] and a list/dict to env["SHLIBPREFIX"]. If there are custom __getitem__/_setitem__ those cases would be covered too.

[mwichmann]

The relevant class is actually SubstitutionEnvironment, and of course there's already some "magic" hiding there.

[mwichmann]

Just note that in the past this was considered performance sensitive stuff... the __setarttr__ may not want to grow lots of checks.

[mwichmann]

So could some variant of Typing help? Right now SCons has a place where it's somewhat aware of types of things, that's in ParseFlags, where there's a dictionary of knows consvars the method might distribute values to. A subset:

    'CPPFLAGS'      : CLVar(''),
    'CPPPATH'       : [],

That doesn't seem too wildly different from:

CPPFLAGS: CLVar = CLVar('')
CPPPATH: list = []

If that information is somewhere, the "checker mode" (or a separate sconslint) could perhaps flag a warning that you just assigned a str to env['CPPPATH']... just thinking out loud, not sure how viable that is applied to dictionary key/values.

[mwichmann]

Just dropping this here to track an issue that isn't decided yet - modifications to env's in Action functions seem like a bad idea. Is this something a checker could detect and maybe warn about?