[Bug] grass.app.runtime.RuntimePaths default value is dangerous and leaks into rendered docs

[echoix]

Describe the bug

The __init__ of class grass.app.runtime.RuntimePaths uses a dangerous default value, and it is already flagged by Pylint W0102. They also leak in the generated docs.

grass/python/grass/app/runtime.py

Lines 28 to 36 in 84ab511

	class RuntimePaths:
	"""Get runtime paths to resources and basic GRASS build properties
	The resource paths are also set as environmental variables.
	"""
	def __init__(self, env=os.environ):
	self.env = env

This mutable default value behavior was the first real Python problem that bit me years and years ago, and made me interested on how Python works. It is easy to get affected and not understand why things don't work.
That pylint warning is not enforced yet, as we have tons of places where we use an empty list for example. See the Pylint W0102 docs to understand the effect and the kind of fix needed.

The dangerous part here for us is that the contents of the env vars are reproduced in the rendered html docs, and change for each run.
We were lucky the env var GITHUB_TOKEN or other more sensitive info didn't show up in the published docs, as shown in the screenshots below.
There's also the problem of systematically have differences in the content, even if unchanged.

To reproduce

1. Generate sphinx docs or download the mkdocs-site artifact of any documentation.yml workflow.
2. Go to the docs for grass.app.runtime.RuntimePaths, or the published https://grass.osgeo.org/grass-devel/manuals/libpython/grass.app.html#grass.app.runtime.RuntimePaths
3. See leaked env vars in docstring

Expected behavior

The environment vars are not leaked systematically

Screenshots

Always different values:

System description

• Operating System: Linux Ubuntu 22.04 on GitHub actions CI, in the documentation.yml workflow
• GRASS GIS version: 8.5.0dev, at least in the main branch from 76bbc71, but probably ever since build: refactor grass.py in preparation for FHS #5933 two weeks ago.

Additional context

[wenzeslaus]

See #6101. The sad part about this is that I noticed that during review, but was not sure if it is really bad or just against the practice we used elsewhere, so I didn't want to push it. I didn't think a computed value, not the code, is what is rendered into the documentation.

[echoix]

See #6101. The sad part about this is that I noticed that during review, but was not sure if it is really bad or just against the practice we used elsewhere, so I didn't want to push it. I didn't think a computed value, not the code, is what is rendered into the documentation.

Indeed, sphinx is not static, but there are some options that can change a bit the behavior. Griffe, the backend for mkdocstrings is by default more static. In any case, it’s a problem even if we ignore the docs part, and even if we ignore the fact that it’s env var. The bad practice (because of real consequences) is present in multiple places in the code

[nilason]

Nasty side effects!

[nilason]

I would consider that a bug of the doc generating software!

[echoix]

Nasty side effects!

I would consider that a bug of the doc generating software!

It’s written everywhere that the modules must be importable and be careful of side effects when importing. If for example you have some code that calls rm -rf / in an __init__.py or anywhere not inside a function (assuming before the protections to not remove the root tree), well they’ll be executed (as expected). Then one would go on Google and search how to undelete files deleted on the command line on Linux…

The option in sphinx that concerns this is autodoc_preserve_defaults https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_preserve_defaults, but would be changed for the whole project. And sometimes, it’s useful as typing annotations can make use of variables representing a commonly used typing annotation that can be written just once at the top of the file.