Delete the `LandMine` class

[AlexWaygood]

@erlend-aasland, what do you think about this? It simplifies some of the typing, since now CConverter.function is always an instance of Function, instead of sometimes being an instance of LandMine. I'm not sure why we need to temporarily set self.function to be an instance of LandMine -- I feel like we can get the same effect by just not setting self.function until after converter_init() has been called. But I'm a bit wary of Chesterton's Fence here -- I'm not sure I understand fully exactly which problem the self.function = LandMine() thing was originally supposed to solve.

[erlend-aasland]

I don't think this is correct. The LandMine class is needed for custom converters. We don't have a lot of coverage for those; we should fix that. I was able to trigger the LandMine class last year when experimenting with custom converters for sqlite3, IIRC.

I'm sure we can fix it with a special case of Function, though; it does not have to be a separate class. And I think we should rename it to something nicer; land mines are not nice.

[AlexWaygood]

Hmmm, I'm still not quite sure I fully understand the situation where setting self.function to be a LandMine provides more safety than just not having self.function be set at all yet. But I'll trust you that it's needed!

I'm sure we can fix it with a special case of Function, though

Yes, maybe it can be a subclass of Function? DummyFunction, on which any attribute access raises AttributeError? That would also mean we could get rid of the assertions we added for mypy.

land mines are not nice

indeed

[erlend-aasland]

I have a test ready. Let's add that first :) You may be right; it could be sufficient to set self.function = None.

[erlend-aasland]

See https://github.com/python/cpython/pull/107496

[AlexWaygood]

it could be sufficient to set self.function = None.

That wouldn't improve anything from a typing perspective: self.function would just change from having a Function | LandMine type to having a Function | None type. We'd still need the assert isinstance(self.function, Function) assertions.

What I'm suggesting is just not having self.function be set at all until after converter_init() has run, so that accessing self.function just raises AttributeError if you try to do it inside converter_init().

We could possibly add a custom __getattr__ method to the CConverter class so that it gives a nice error message when the AttributeError is raised inside converter_init() and the attribute is "function"

[erlend-aasland]

Aha, I misunderstood you. Yes, I like that.

[AlexWaygood]

Thoughts on the updated version?

[erlend-aasland]

Thoughts on the updated version?

It's nice. I think it would be helpful to include the attr name in the error message, since that can help debugging.

[AlexWaygood]

It's nice. I think it would be helpful to include the attr name in the error message, since that can help debugging.

Hmm. Now the attribute error is raised trying to access self.function itself rather than attributes of self.function, so that's somewhat harder. But maybe we could use fail() inside the __getattr__ method (instead of just raising AttributeError) to indicate the precise line number in the clinic input that triggered the error?

[erlend-aasland]

Hmm. Now the attribute error is raised trying to access self.function itself rather than attributes of self.function, so that's somewhat harder. But maybe we could use fail() inside the __getattr__ method (instead of just raising AttributeError) to indicate the precise line number in the clinic input that triggered the error?

Hm, true. Calling fail() inside __getattr__ is a possibility.

(BTW; the line numbers in the tests messed up because of FakeClinic; that's why most of the tests report the line number as 0.)

[erlend-aasland]

Let's take this to the cpython repo :)

[erlend-aasland]

By the way, one of the comments mention that the LandMine class is there to protect against certain types of cloning. We should try and add a test for that case as well; it may take a slightly different path, so we need to make sure we catch that (potentially) slightly different scenario.

[erlend-aasland]

By the way, one of the comments mention that the LandMine class is there to protect against certain types of cloning. We should try and add a test for that case as well; it may take a slightly different path, so we need to make sure we catch that (potentially) slightly different scenario.

Re-reading that comment, it kinda looks like we're covered. BTW, here's the commit that introduced it: 7726ac9163081a3730d30d4334135d6bf26900fc

[AlexWaygood]

here's the commit that introduced it: 7726ac9

whelp, there's a lot going on there

[erlend-aasland]

whelp, there's a lot going on there

IIRC, they used to queue approved commits, and them squash them all with a giant change log entry and/or git commit message. There's a lot of commits like that from the way old development workflow :)

[AlexWaygood]

I filed https://github.com/python/cpython/pull/107541