Avoid crash when passing dict to dependencies

[eyalitki]

While there are good type checks on the "dependencies" addition, they are located after a caching logic that assume the object is hashable. Ensure the caching logic will be skipped if the (probably invalid) object is not hashable.

Resolves #13539.

[bruchar1]

I think this should be detected at the interpreter level. This function should not be allowed to receive objects of the wrong type.

[eyalitki]

The code flow is:
interpreter.py -> build.py : BuildTarget() -> process_kwargs() -> add_deps()

And it seems that all of the type checks are done inside the process_kwargs() function, even including the simple check that link_args is a string. build_target() in interpreter.py has practically no type checks, on any of the fields.

On top of that, add_deps() does include the needed type checks (as mentioned in the commit message), yet the caching is done prematurely hence passing a dict will cause a crash (as reported in the bug).

[eli-schwartz]

I think @bruchar1 was trying to point out that there is a general goal to move all argument checking out of various inconsistently located places and move them to the newer model that looks like:

@typed_kwargs(
    'funcname',
    KwargInfo('link_args', ContainerTypeInfo(list, str)),
)

which then raises an error at the interpreter level if you define a meson.build with link_args: {} before build.py is reached.

We used to do what we do here for process_kwargs, with all meson functions. Most of them have moved to using the @typed_kwargs decorator in the interpreter, and then guaranteeing that build.py always gets data of the right general shape. BuildTarget is a holdout because it has some of the most complicated code of all.

[eyalitki]

OK, now I better understand.

That said, this commit is a bug fix for a crash and (aside from practically being my first commit to your project) wasn't intended to be a massive refactor/cleanup of what you describe as "some of the most complicated code of all".

How do you usually manage such cleanup tasks? Should there be a dedicated issue for it? Sounds like it would require quite a big pull request.

[bruchar1]

I don't think a refactor is needed. The build targets are already mostly typed. I think the bug is that the 'dependencies' keyword is missing. The fix is probably as simple as adding DEPENDENCIES_KW to _ALL_TARGET_KWS in type_checking.py

[eli-schwartz]

That can be handled by "allowing" the bad type in the list of types and combining it with the validator= argument to KwargInfo which does a fine-grained check with the ability to emit custom error descriptions.

[eyalitki]

This might work for the BuildTarget type, but not for the complex "else" case that was in the original logic for add_deps() which wasn't typed but used some hackish workaround.

If it is OK with you, I prefer to leave the current pull request with the addition of type checking as suggested. If these checks could be later refined to improve usability (including reverse engineering the original "else" case) than this will obviously be welcome, but I'm not sure this is a change that fits my current understanding of the internals of this project.

I'm not sure how do you track such tasks (if under "issues" or somewhere else). I'm still new to working with this community (happily using your project for years now) and trying to get the hang of how things are working from a procedural standpoint.

[eyalitki]

OK, I take back what I said above. While the suggested type check passed the local unit test run, it fails in the C/I checks, in what looks like reliance on the place holder logic from the else case in add_deps(). As such, I believe that the right approach is to use the 1st version of the commit, and track the migration of this code to proper type checking under a separate issue.

Please let me know how do you prefer to continue.

[bruchar1]

I think having the checks at the typed_kwargs level is the right thing to do. Having specific error messages when BuildTarget or Subproject are given as 'dependencies' is probably a good thing, and could be done using a validator, as @eli-schwartz suggested. However, in that case, you must be careful because the DEPENDENCY_KW is already used for declare_dependency.

About the failing CI, it seems to simply be that modified error messages should reflect in some tests.

[eyalitki]

If I understand correctly, the suggestion is to add DEPENDENCY_KW in the the type checking because it already exists, yet to:

• Extend it to additional types + validators to have nice errors on these additional types (otherwise the tests fail)
• While this is doable for BuildTarget it is unclear for other types as they weren't even named as types in the original add_deps() code
• Do so probably using evolve so not to affect the original DEPENDENCY_KW that is also used in declare_dependency, which practically means duplicating it as the original is only 5 lines long.

Please accept my apology, but for every step of me following your instructions it just seems to fail in the C/I and end with more instructions that were initially missed. The change has already grown beyond my understanding of your project, I don't believe that this ongoing review process is cost effective (especially given the rest of my submitted pull requests that weren't even reviewed in the last several days) and I don't have the capacity to invest what seems to be significant additional resources on the pull request.