Fix the use of setup's '--profile-self' path … #11934
Conversation
mesonbuild/msetup.py
Outdated
| 'capture=capture,' | ||
| 'captured_compile_args_per_buildtype_and_target=captured_compile_args_per_buildtype_and_target)', globals(), locals(), filename=fname) | ||
| captured_compile_args = locals()['gen_result'] | ||
| assert ((captured_compile_args is None) or isinstance(captured_compile_args, dict)) |
There was a problem hiding this comment.
This is not a real assert, which may explain why mypy didn't do type narrowing on it.
There was a problem hiding this comment.
That's incredibly helpful.
It looks like an assert, it parses/interprets without any complaint, and it also behaves correctly -
>>> assert (({} is None) or isinstance({}, dict))
>>> assert ((None is None) or isinstance(None, dict))
>>> assert ((123 is None) or isinstance(123, dict))
... AssertionError
So would you care to share your insight on what makes this not a real assert?
There was a problem hiding this comment.
Let's rewrite this assert to be semantically identical, but more easily readable.
isnone: bool = captured_compile_args is None
isdict: bool = isinstance(captured_compile_args, dict)
test_statement: T.Tuple[bool] = (isnone or isdict, )
assert test_statementThis doesn't generate the same bytecode, but the only difference is that it needs to STORE_NAME before the assert.
There was a problem hiding this comment.
(I think the other issue is that mypy can't do type narrowing on or statements, but can do it when each is a separate statement.)
There was a problem hiding this comment.
We have quite different definitions of easily readable 😄
Even with the those contortions to seemingly try to spoon-feed mypy, it still thinks we'll can end up returning an Any type... But I suppose I (and now you also) must be misunderstanding the assert and/or not providing sufficient information to mypy to deduce narrow the type with the assert.
Also, you haven't explained why this is not a real assert. As far as I can see, everything about it does what it should and that all signs point to a limitation with mypy's type deduction , but I'd still love to learn what makes it an incorrect or not real assert.
There was a problem hiding this comment.
054e714 seems to be enough of a dance, to get mypy to figure things out.
mesonbuild/msetup.py
Outdated
| @@ -308,7 +312,8 @@ def _generate(self, | |||
| os.unlink(cdf) | |||
| raise | |||
|
|
|||
| return captured_compile_args | |||
| # Even with the asserts, mypy's type checking thinks this could be 'Any' type, not the None|dict we know it to be, hence the suppression comment - | |||
| return captured_compile_args # type: ignore[no-any-return] | |||
There was a problem hiding this comment.
If you're suppressing errors there's a 99% chance you misunderstood how things work. In the remaining 1% case you should have a better comment than "I thought this was supposed to work but it doesn't so just ignore it" to explain why.
There was a problem hiding this comment.
That's intriguing. Supposing we follow through your 1% case scenario, where there actually is no misunderstanding and we actually have given mypy everything needed to deduce the type that it's getting wrong: You say you don't like a comment that explains why we think mypy has enough to deduce a type but which it fails to deduce. So what are you proposing would be a better kind of comment in this scenario?
There was a problem hiding this comment.
The problem is "but which it fails to deduce".
If you've given it everything it needs to deduce the type, but it isn't deducing it, then either the code isn't giving it the information you think it is, or else the mypy documentation should discuss the caveat and serve as a reference, or else it's a bug in mypy and deserves to be reported.
(Either way using a cast is often better.)
There was a problem hiding this comment.
Well, this statement would be a lot easier to swallow if even just 1 of the currently 30 instances of # type: ignore that are scattered throughout meson had any hint of an accompanying explanation and/or apology, never even mind their total absence of any reference to a mypy bug that, according to you, must be used if one has to resort to suppressing a mypy type warning for something that otherwise definitely does provide enough type info, that leaves a mypy bug as the only remaining conclusion.
There was a problem hiding this comment.
Ha. Well, I've found 1 instance of this in mtest.py, ln 126.
There was a problem hiding this comment.
In this case it seems the solution was to annotate the original value as not just None, but T.Optional[TheOtherThingItCouldBe], so probably not a mypy bug, and not even a dance. This is the problem with being quick to mark a test as "expected to fail", especially in newly added code rather than when adding tests after the fact.
e.g. arglist.py
TODO: fix 'Invalid index type' and 'Incompatible types in assignment' erros
which was added in a PR that exclusively added typing to existing code, and inherently has more lax standards; in the generic case, when adding tests to existing code, it's okay to mark them as expectedfail "but we do not know why".
There was a problem hiding this comment.
See commits 1 and 3 here: #11938
I added comments to 3 cases, and removed 3 more entirely because it turns out we can check the mixins.
There was a problem hiding this comment.
Many of those mypy work-around excuse comments you quote look very much like this scenario, by which I mean they certainly don't reference a mypy bug that you eventually said is what should be used to accompany these suppressions.
This -
Mypy doesn't follow hasattr, and it's pretty easy to visually inspect that this is correct, so we'll just ignore it.
describes a very similar scenario to what's been going on here. In my case, mypy doesn't follow some pretty clear to see type narrowing deduction in what is (until anyone suggests otherwise ... i.e. I'm still waiting; are you still insisting it's not a real assert?), valid and clear python.
My impression is that it's actually quite misleading and unhelpful to say "there's a 99% chance you misunderstood how things work". Seems like it's more a case of: mypy's type checking can simply be wrong, but with some persistence and wrangling/restructuring - perhaps even contortions 😄- of the code, it might be possible to get mypy to figure things out.
In future, and especially if it can be explained in a sentence or two, if you have insight on what you think is a misunderstanding, there's a 100% chance that it'll be more useful and productive to share that insight, rather than leaving nothing more than a, "That's not a real assert" kind of remark. This whole exchange could have been far simpler if you'd have just said something like -
"It looks like you're not giving enough type info to mypy. Here's why I think this: ..."
or -
"Use of '# type: ignore' should be a last resort if you think you've given mypy enough to go on, [which it looks like you haven't because ...] / [which it looks like you have but I can't suggest what more you could do to help it out]. Any last-resort suppressions should be accompanied with a good explanation and preferably (but not actually required, as evidence by the many existing unaccompanied uses) a comment pointing to a mypy bug".
There was a problem hiding this comment.
so probably not a mypy bug
And by the way, if it's valid code from which the reader can trivially deduce type-correctness, then I'd say it very much is a mypy bug. Call it a limitation if you prefer, but a work-around from valid, type-correct code is still a work-around.
There was a problem hiding this comment.
describes a very similar scenario to what's been going on here. In my case, mypy doesn't follow some pretty clear to see type narrowing deduction
The difference is that not everything is, in fact, a type narrowing deduction:
- mypy literally does not implement hasattr and is expected to get it wrong for the reason in the comment
- whereas it is "supposed" to type narrow on isinstance / x is None and was simply failing to do so, here, and the comment is saying (paraphrased):
this code is correct and mypy supports it and correctly analyzes it. Except actually it does not and I'm not sure why
preferably (but not actually required, as evidence by the many existing unaccompanied uses) a comment pointing to a mypy bug
But also I didn't insist on an url comment, I merely said that there should exist references. I had intended to mean, by that, that it should be possible to read the mypy documentation / bugtracker and conclude some authoritative fact like "mypy does not support hasattr, so we don't even try to type narrow" or "this is definitely a mypy bug because pylance gets it correct, thereby proving the annotation is not faulty" or "this is a bug, here's an url link describing why". Apologies for the confusion.
Again, this is no different from e.g. a unittest. If a PR adds code that causes a unittest to fail, and you want to mark the unittest as "expectedfail", I expect some form of explanation for why the unittest failure isn't a problem, and that explanation cannot be "I believe I did everything right, the test still fails, it must be a problem in the test software". But "I definitely 100% can verify that the test software is buggy because it doesn't know how to frobnicate the wolpertinger and this test does that" would be a good explanation.
If in doubt, I would prefer that rather than "use type: ignore as a last resort" / "mark this test as expectedfail" a self-review comment is made saying "this test fails and I'm not sure why, any ideas?"
After the recent change to the backend `generate(...)` signature for
'--genvslite', `profile.runctx('...')` needs to construct the correct
cmd string and also extract the return result from `locals()`.
|
Thanks, merged. n.b. https://cbea.ms/git-commit/ (Commits manually squashed and commit message reformatted to match the blogpost guidance.) |
... after recent change to the backend 'generate(...) ' signature for '--genvslite': profile.runctx('...') now constructs the correct cmd string and also extracts the return result from 'locals()'.
This is in reference to this PR comment
mypy's type-checking was complaining that this '_generate(...) -> T.Optional[dict]:' func now might be returning the Any type, after, I presume, seeing me assign the result out of the 'locals()' dictionary to 'captured_compile_args'. I thought mypy is supposed to deduce type narrowing with 'asserts' and 'isinstance', but that doesn't seem to be the case here 🤷♂️ so I've had to add a mypy type check error suppression comment on the return line.