Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

universal.c - handle version style imports as a version check #21279

Open
wants to merge 2 commits into
base: blead
Choose a base branch
from

Conversation

demerphq
Copy link
Collaborator

if someone does

use Thing 1.234;

it is interpreted as

BEGIN {
    require Thing;
    Thing->import();
    Thing->VERSION(1.234);
}

however in the case of

use Thing "1.234";

we treat it as

BEGIN {
    require Thing;
    Thing->import("1.234");
}

however if people use Exporter::import() as their
importer then its import will turn such cases silently into

Thing->VERSION("1.234")

anyway.

With the new logic to detect if someone has called into
UNIVERSAL::import() with an argument we were not discriminating between
the two cases.

This patch basically does the same thing that Exporter would.

Note this patch doesn't have tests right now, however it is being implicitly tested by various code in the cpan/ directory. I will add them later when i have more time. I wanted to get this pushed ASAP as the core point is causing issues in #21269

We were displaying the start of the string wrong when the difference
started before offset 40.
if someone does

    use Thing 1.234;

it is interpreted as

    BEGIN {
        require Thing;
        Thing->import();
        Thing->VERSION(1.234);
    }

however in the case of

    use Thing "1.234";

we treat it as

    BEGIN {
        require Thing;
        Thing->import("1.234");
    }

however if people use Exporter::import() as their
importer then its import will turn such cases silently into

    Thing->VERSION("1.234")

anyway.

With the new logic to detect if someone has called into
UNIVERSAL::import() with an argument we were not discriminating between
the two cases.

This patch basically does the same thing that Exporter would.

This patch also special cases the class Test::SubExport::SETUPALT
so that Sub::Exporter does not break, as it is very high in the CPAN
river, and when it is broken we have problems with test reporting.
@karenetheridge
Copy link
Member

I agree with @haarg on "I hate this", and would prefer to find a migration path away from this internal hack.

How about something like:

  • we warn, both internally and in Exporter, when a stringy version is passed
  • but propagate the call to VERSION, for now, in both (as you're doing here)
  • with a plan to remove this behaviour (in both) with the usual deprecation schedule

@mauke
Copy link
Contributor

mauke commented Jul 22, 2023

I agree with @karenetheridge that a warning seems right here. I'm less sure about changing Exporter; it's been doing it for decades now, and this is documented behavior ("Module Version Checking").

@karenetheridge
Copy link
Member

karenetheridge commented Jul 22, 2023

Exporter::import doing something differently than native import seems like a misfeature that should really be removed IMO, but it needs to warn in the interim since the impact is definitely not zero.

I don't think we should change the internal import to act like Exporter::import; it could also simply warn, rather than being fatal (while keeping the same behaviour of not propagating the arg to VERSION, just as in 5.38.0.)

@demerphq
Copy link
Collaborator Author

@ether given how even experienced devs like yourself and myself can make this mistake and that it is in core code already for instance and I know I've seen it discussed on Perlmonks long ago and the frequency we are seeing it come up when Exporter is not used, I suspect we will see it very broadly in the wild.

Personally I don't think that you and @haarg hating the idea that Exporter supports version checks via import is a good justification to change such longstanding and likely broadly used code.

I think if you really want to deprecate Exporter doing this you should find out what kind of impact that would actually have and demonstrate its not going to be a huge can of worms. As is, the new exception in UNIVERSAL::import is revealing that people do stuff if they can get away with it a lot more than might be expected. I suspect that string version checks via Exporter to be even more broadly used than no-op import calls.

I think before we talk about deprecating that behavior (instead of for instance blessing it) we should have a good idea how much code would be impacted. If it is not widely used then sure why not deprecate it? But vice versa if it's widely used then we should do the opposite, and bless it. We shouldn't cause widespread bitrot on cpan lightly, and imo your and @haarg's preference isn't a compelling reason. At the very least I think it's a reasonable thing to do, and obviously so did the person who added it to Exporter as did the pumpking who signed off on the change back in the day, which is at least two against and two in favor, canceling each other out. In such a scenario and absent a good technical reason, why should your preference prevail over mine or that pumpkings?

There are lots of things different things that core devs might wish we hadn't done, but we did, and IMO we should have a better reason than "I hate this" to change it so long after if that change would cause a lot if disruption?L

The new exception is at least motivated by a desire to fix a long time issue on case insensitive file systems, but even there I am starting to question if that capability is worth the cost. What is broken by Exporter::import supporting version checks and what would break if UNIVERSAL::import supported it also? There has to be more than "I hate it" to justify a change that impacts lots of CPAN. We should be fixing a real problem or achieving a valuable goal or something like that.

Anyway, feel free to "run the numbers" somehow to come up with an estimate of what impact your deprecation proposal would have. That would allow us to continue this discussion in a more informed manner.

@demerphq
Copy link
Collaborator Author

demerphq commented Jul 22, 2023 via email

@haarg
Copy link
Contributor

haarg commented Jul 23, 2023

I think we may need to relax the error into a warning in all cases, given the number of failures it is causing that are unrelated to trying to check a version. Possibly go through the normal deprecation process.

@demerphq
Copy link
Collaborator Author

demerphq commented Jul 23, 2023

I think we may need to relax the error into a warning in all cases, given the number of failures it is causing that are unrelated to trying to check a version

Yes, indeed. Although I expect that will only reduce the volume of these issues, not eliminate them.

Possibly go through the normal deprecation process.

With regard to deprecation do you mean Exporter supporting version checks? If so, I don't think that is settled at all and I am in opposition to it. I think actually that we should do the opposite and bless it as something that import()ers /should/ do. We will have far less fallout from it, and it will DWIM more than the current logic does. After all if an exiting import()er implementation doesn't support it then the user should immediately find out via an error. Whereas removing it from Exporter would have broad fallout.

As far as I can tell the change to make Exporter validate versions goes back to perl 5.001, in 1995, in e50aee7 by @doughera88. Making the behavior 28 years old, and signed off by Larry Wall himself. I really don't think we can deprecate such a case without really good basis to do so, and I don't think we actually have such a basis given we dont have a stack of bug reports related to it. (Although I do know we have had bug reports that NOT using Exporter doesn't do the same thing as Exporter would, IOW bug reports that support the other position.) Obviously Exporter supporting it doesn't cause carnage, so it seems there is no actual technical problem it causes. We make fairly strong backwards compatibility commitments, and I think deprecating this feature from Exporter without a clear technical justification would violate the spirit of those commitments and open up a contentious debate that we should avoid.

@haarg
Copy link
Contributor

haarg commented Jul 23, 2023

No, I meant the "undefined import ignores its arguments" behavior. Warning for a couple releases before turning it into a fatal error seems reasonable, given the amount of code that has broken from the change. And of the things broken, so far I've seen less due to versions than things like use_ok.

I do wish we could remove the behavior from Exporter, but I don't really think that is viable at this point.

If we wanted use Foo '1.0'; to work as a version check, the proper place to implement that is the parser, passing the value to VERSION, not in an import method.

I am strongly opposed to blessing "version as an import argument" as expected behavior for all import methods, which would be the result of implementing it in UNIVERSAL::import.

@demerphq
Copy link
Collaborator Author

No, I meant the "undefined import ignores its arguments" behavior. Warning for a couple releases before turning it into a fatal error seems reasonable, given the amount of code that has broken from the change. And of the things broken, so far I've seen less due to versions than things like use_ok.

Sure, I can look.into that.

I do wish we could remove the behavior from Exporter, but I don't really think that is viable at this point.

Agreed.

If we wanted use Foo '1.0'; to work as a version check, the proper place to implement that is the parser, passing the value to VERSION, not in an import method.

Also agreed. Although I'd add that I think we should do this.

I am strongly opposed to blessing "version as an import argument" as expected behavior for all import methods, which would be the result of implementing it in UNIVERSAL::import.

I am strongly opposed to people saying they are strongly opposed to proposals being discussed in our community without explaining themselves. It makes it difficult to move forward by establishing consensus through well intentioned debate. Perhaps if I understood why you oppose it then I might alter my opinion, perhaps I could provide a counter argument that would lead you to change your opinion. When there is no explanation the implication is that there is no room for debate or consensus building nor respect for alternate opinions, which I find quite frustrating and demotivating.

@karenetheridge
Copy link
Member

karenetheridge commented Dec 25, 2023

I think if you really want to deprecate Exporter doing this you should find out what kind of impact that would actually have and demonstrate its not going to be a huge can of worms. As is, the new exception in UNIVERSAL::import is revealing that people do stuff if they can get away with it a lot more than might be expected. I suspect that string version checks via Exporter to be even more broadly used than no-op import calls.

We are now warning on this usage, via commit f1cf82e, and many issues that were revealed have now been fixed (use_ok being a major one, thank you @haarg for sending out lots of patches for this) -- was the impact as widespread as we had feared? Can we now more confidently move forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants