Major rewrite of the rewriter and the static introspection tool - #12277
Conversation
ed089ca to
25c3cf2
Compare
|
Thank you for working on this. I will try to have a deeper look and to test it soon. For the tests, I think the best thing to do is to ensure that each bug you found is covered by a test. I wonder if this could be splitted into multiple commits, to ease understanding what each part fixes, especially for trivial fixes that are not directly part of the refactor. |
I should have done that everytime it found a bug. I cannot do it now, since I forgot most of the bugs.
I will do that for "trivial fixes that are not directly part of the refactor" , but the rest will still be in one big commit since it is hard/impossible to split. |
7425a57 to
b594817
Compare
|
I splitted the trivial fixes intro seperate commits. |
bruchar1
left a comment
There was a problem hiding this comment.
I just did a quick pass on python syntax. I didn't tried the functionality yet.
| return SymbolNode(Token('', '', 0, 0, 0, (0, 0), val)) | ||
|
|
||
| class DataflowDAG: | ||
| src_to_tgts: T.DefaultDict[T.Union[BaseNode, UnknownValue], T.Set[T.Union[BaseNode, UnknownValue]]] |
There was a problem hiding this comment.
There is a lot of T.Union[BaseNode, UnknownValue]. Maybe should you create a type for that? e.g. NodeOrUnknown = T.Union[BaseNode, UnknownValue].
There was a problem hiding this comment.
I'm against that. It is less readable and only marginally shorter.
| active = set(srcs) | ||
| while True: | ||
| if reverse: | ||
| new: T.Set[T.Union[BaseNode, UnknownValue]] = set() |
There was a problem hiding this comment.
The new set is created on both branches of the if. I think you could initialize it before the if.
| 'conditional', | ||
| ] | ||
| result += [{k: v for k, v in i.items() if k in keys}] | ||
| result += [{k: v for k, v in i.__dict__.items() if k in keys}] |
There was a problem hiding this comment.
This __dict__ seems a code smell... Is it just because mypy do not detect the type of i correctly?
There was a problem hiding this comment.
I think you could rewrite this as:
result += [{k: getattr(i, k) for k in keys}]| intro_types = get_meson_introspection_types() | ||
|
|
||
| if 'meson.build' in [os.path.basename(options.builddir), options.builddir]: | ||
| # TODO: This if clause is undocumented. |
There was a problem hiding this comment.
That is out of scope for this PR. Also, I don't like the command line api here:
I think that meson introspect meson.build --targets should be changed to one of these:
meson static-introspect . --targetsmeson static-introspect --targetsmeson rewriter --targets
I don't like the fact that meson introspect meson.build --targets and meson introspect builddir --targets are radically different things, but the cli makes it look like they are two just two ways for meson to find the correct directory.
|
|
||
| if 'meson.build' in [os.path.basename(options.builddir), options.builddir]: | ||
| # TODO: This if clause is undocumented. | ||
| if os.path.basename(options.builddir) == 'meson.build': |
There was a problem hiding this comment.
There is environment.build_filename constant. Maybe should you use it?
| if 'meson.build' in [os.path.basename(options.builddir), options.builddir]: | ||
| # TODO: This if clause is undocumented. | ||
| if os.path.basename(options.builddir) == 'meson.build': | ||
| sourcedir = '.' if options.builddir == 'meson.build' else options.builddir[:-11] |
There was a problem hiding this comment.
and use -len(environment.build_filename) here...
| res = [root_dir / i['subdir'] / x for x in res] | ||
| res = [x.resolve() for x in res] | ||
| return res | ||
| def list_targets_from_source(intr: IntrospectionInterpreter) -> T.Any: |
There was a problem hiding this comment.
Instead of making this Any, it could at least be T.List[T.Dict[str, object]]
| 'conditional', | ||
| ] | ||
| result += [{k: v for k, v in i.items() if k in keys}] | ||
| result += [{k: v for k, v in i.__dict__.items() if k in keys}] |
There was a problem hiding this comment.
I think you could rewrite this as:
result += [{k: getattr(i, k) for k in keys}]| # ignores ParanthesizedNode, the binding power of the inner node is | ||
| # relevant. | ||
| return precedence_level(node.inner) | ||
| raise TypeError |
There was a problem hiding this comment.
Usually you would use RuntimeError, but we have MesonBugError (or maybe it's MesonBugException?). I think we should use that here.
| else: | ||
| for for_machine in [MachineChoice.BUILD, MachineChoice.HOST]: | ||
| self._add_languages(args, required, for_machine) | ||
| return UnknownValue() |
There was a problem hiding this comment.
The explanation for UnknownValue was for cases where we couldn't know what would be returned. I'm not sure why I follow that add_languages returns UnknownValue and not bool. If it returns it will always return a bool (or it will abort, but for the purpose of the rewriter that doesn't really matter, does it?)
There was a problem hiding this comment.
var = add_languages('rust', required: false)
message(var)
prints true on some machines and false on others.
So func_add_languages has to return UnknownValue.
There was a problem hiding this comment.
It returns a boolean? I'm just not understanding here, if add_languages(), which returns a bool cannot be statically determined, then what can? There are no functions I know of that always return the same value in every case.
There was a problem hiding this comment.
You seem confused, let me clear that up:
You know that if you run meson setup builddir, the contents of the resulting builddir-directory depend on
- The contents of meson.build
- The machine you are using.
If for example both you and I clone the same project that is using meson, and we both run meson setup builddir, those two directories are not (necessarily) identical if we have different machines. This is not a bug, this is intentional. Therefore, if we both run meson introspect builddir, we (might) get different results.
But if we both clone the same project and run meson introspect meson.build we get the same result. If we don't, that is a bug. In other words, the job of ast/introspection.py is to know what happens if we run meson setup on a different, unknown machine. In other words, ast/introspection.py has full knowledge about the contents of meson.build, but no knowledge about the build/host/target machine. Let's say meson.build contains:
srcs = ['1.c']
srcs += files('2.c')
if 3+4 == 7
srcs += '3.c'
endif
if build_machine.system == 'linux'
srcs += 'linux-specific.c'
endif
if add_languages('rust', required: false)
srcs += 'rust.rs'
endif
executable('foo', srcs)
If I run meson introspect meson.build on my machine, the job of meson is to figure out what sources belong to the foo-executable file if you run meson setup on your machine.
It knows that the foo-executable contains the sources 1.c and 2.c. It does not know that it contains '3.c', since I was too lazy to implement that. There is no way for it to know whether the foo-executable contains the sources 'linux-specific.c' or 'rust.rs', since a program running on my machine cannot know whether your machine has a rust compiler installed.
There was a problem hiding this comment.
Looking at this again, my concern here is that there seems to be a conflation between the type a function returns and whether we can know what value that type is.
Like in the case of add languages above, we know that it returns a bool, we just don't know whether that bool is true or false.
This leads me to wonder whether making UnknownValue generic would be of benefit. Because as you point out, add_languages with required : false will return different values depending on whether rustc is installed. but it won't return a list, or a string, or 7, it will only return true or false. Even with this, there may well be times where we say UnknownValue[TPYE_var] because we can't be more specific, but that still is something.
There was a problem hiding this comment.
It might improve the typing, but it would only be used for static analysis. At runtime you'd still have UnknownValue().
| def function_call(self, node: mparser.FunctionNode) -> T.Optional[InterpreterObject]: | ||
| func_name = node.func_name.value | ||
| (h_posargs, h_kwargs) = self.reduce_arguments(node.args) | ||
| (h_posargs, h_kwargs) = self.reduce_arguments(node.args, include_unknown_args = True) |
There was a problem hiding this comment.
No spaces around the = operator
ee79d23 to
79661ac
Compare
|
Cloned this branch, ran Python 3.11.5. |
Confirmed and on my way to fix it. |
Fixed in b98292e |
b98292e to
d5a95d6
Compare
|
Any update? |
|
Hi! Would you be able to rebase the PR? |
I could but only if a maintainer says that it gets merged after rebasing. |
|
Not a maintainer but I will try to give it a quick review beforehand, at least. |
|
It looks sane. It's a huge commit, but that can be fixed after conflicts are resolved. |
|
We still need someone with merge permissions. |
|
Well as long as it has conflicts no one will look at it. |
To improve type-safety and readability we replace a dictionary with a new class `IntrospectionDependency`.
The AstInterpreter now stores how deep into if/elif/else we are. This is currently dead code, but it will be read in future commits.
83b9549 to
960e8a9
Compare
|
@Volker-Weissmann then I'm perfectly happy. I personally like introducing tests with an expected fail status and then fixing them (see the Fortran dependency scanner work), I think there is value in seeing "Look, this doesn't work" and then "See, I fixed it" |
960e8a9 to
e8ba9e1
Compare
|
Failure seems unrelated |
bruchar1
left a comment
There was a problem hiding this comment.
There is something that bothers me about UnknownValue. it seems sometime it represents a Node, and sometime it represents a value. Would it be possible to have an UnknownNode deriving from BaseNode? That way, it would simplify typing, and remove that node vs value confusion.
| raise TypeError | ||
| return ret | ||
|
|
||
| def get_cur_value(self, var_name: str, allow_none: bool = False) -> T.Union[BaseNode, UnknownValue]: |
There was a problem hiding this comment.
It seems return type can be None as well
While it would be possible to split It would simplify typing a bit, but since I think just having a type called should be clear to the reader. |
Replace the variable tracking of `AstInterpreter.assignments` with a slightly better variable tracking called `AstInterpreter.cur_assignments`. We now have a class `UnknownValue` for more explicit handling of situations that are too complex/impossible.
Replace `AstInterpreter.reverse_assignment` with `AstInterpreter.all_assignment_nodes`. This does not give us an immediate advantage but will be useful in future commits.
Some of the evaluate_* functions in AstInterpreter seem very broken and do not even evaluate all of the AST. I do not know what the original author thought, so I just fixed it.
Make the AstInterpreter create a directed acyclic graph (called `dataflow_dag`) that stores the how the data flowes from one node in the AST to another. Add `AstInterpreter.node_to_runtime_value` which uses `dataflow_dag` to find what value a variable at runtime will have. We don't use dataflow_dag or node_to_runtime_value anywhere yet, but it will prove useful in future commits.
Without this commit, something like this crashes the static introspection/rewrite tool: ``` default_options : ['warning_level=' + run_command(['echo', '3']).stdout().strip()], ``` This commit does not reintroduce mesonbuild#14382.
`resolve_node` is simply a half-broken, worse implementation of `node_to_runtime_value` that we recently introduced. In the example below, the static introspection tool/rewriter now understands that the name of the executable is foo instead of bar: ``` var = 'foo' name = var var = 'bar' executable(name, 'foo.c') ```
`AstInterpreter.node_to_runtime_value` can now resolve function calls.
De-duplicate some code by extracting the common code into the new `rm_src_or_extra` function.
Change the semantics of IntrospectionBuildTarget.source_nodes and IntrospectionBuildTarget.extra_files . The rewriter and the static introspection tool used to be very broken, now it is *less* broken, hence we add some tests in this commit. Fixes mesonbuild#11763
Without this commit, the static introspection tool crashes when introspecting systemd since certain values are `UnknownValue` which was unexpected. (I tested sytemd's commit hash fefcb935cd.)
e8ba9e1 to
ce02227
Compare
|
@dcbaker with 1.8.1 out of the door let's go |
|
merged |
|
FUCKING FINALLY! THANKS TO EVERYONE WHO HELPED IN THIS PR. |
|
Thank you for being able to push this over the finish line, and sorry I wasn't able to be any help myself in the end :D this is 2500 lines of code I mostly do not understand, so I'm grateful someone else ended up being able to review it. Sorry for flaking out. :( The improvements sound great. |
The rewriter and the static introspection tool used to be very broken, now it is less broken.
The most important changes are:
class UnknownValuefor more explicit handling of situations that are too complex/impossible.the tool now knows that the name of the executable is
fooand notbar. Seedataflow_dagandnode_to_runtime_valuefor details on how we do this.To test my work I wrote a script that:
git clone's a couple of big projects using meson (e.g. systemd).meson introspect meson.build --targetsandmeson introspect build_folder --targetsare not contradicting each other.meson introspect meson.build --targetsis the same for two different versions of meson (the one we want to test and a known good version).meson introspect meson.build --targetsand checks ifdoes not crash and produces the expected output.
I think this script is very useful (it found a ton of bugs), but I do not know where to put it, so it currently only exists on my machine. It is too slow (1 hour iirc, haven't measured) to run it in the CI pipeline. In the docs you write
Is this testing (partially) automated? If so, could we merge my script with it?
@bruchar1 @kcgen Afaik you are one of the few people using the rewriter/static introspection tool in production. Could you test your usecases and voice your opinion?
@eli-schwartz You promised me this in a mail: