Maybe an opt-in flag, and raise an error to specify the flag when trying to access XML::Errors.errors, since we can't access global errors anymore?

I don't like the idea of breaking the existing code. An opt-in means that. I was playing with the idea of using a macro hook to detect the usage of XML::Error.errors and only then build the necessary setup, but method_missing doesn't work on class methods 😩

Suppose we could somehow detect whether XML::Error.errors is ever used and implicitly activate global error collection. That would basically preserve the current behaviour. Existing code using XML::Error.errors continues to build. But that also means we implicitly preserve the memory leak!
Just because a call to XML::Error.errors exists somewhere in the program doesn't tell anything about whether or when it's actually called. A stray call somewhere deep down an unlikely codepath path could mean the method is technically reachable, but it probably won't actually ever run.

I don't think we should do that. If you really want to use XML::Error.errors despite its shortcomings, it should require to explicitly acknowledge the inherent memory issue.

Considering this, I don't see any good reason to continue using XML::Error.errors at all. It was a bad idea in the first place and is inherently broken. It cannot be fixed, so we might as well break it to make this explicit.

I'm not sure we'll be breaking anything:
https://github.com/search?q=%22XML%253A%253AErrors.errors%22+language%253Acrystal

The upgrade path looks easy (use XML::Reader#errors instead of XML::Errors.errors). At worst an opt-in flag would reenable the behavior for a quick compilation fix.

Playing devils advocate, if it's really not used anywhere and is already deprecated, do we really need to do anything at all and just save the break until 2.0? Like sure it's leaking memory but is it actually a significant amount to make going thru all this effort of discussion/implementation worth it?

Yes, this is immensely relevant. This leak was discovered in a production application.

An app that parses large numbers of XML documents will occasionally encounter some parsing errors. All these errors get collected in XML::Error.@@errors. With no clearing, it just piles up.

Actually seems there's still a usage of it in the stdlib in arm context: https://github.com/search?q=%22XML%3A%3AError.errors%22+language%3Acrystal&type=code. The previous search link was not 100% correct.

EDIT2: These are from old forks that aren't in master anymore.

This leak was discovered in a production application.

But isn't the solution for them to just stop using this deprecated API and tada problem solved?

EDIT: NVM, I see the problem now. It'll leak because they're not using it. 👍.

The problem is that the memory exists even when you don't use the deprecated API. There's no opt-in for XML::Error.errors so it always collects all errors.

The example in the OP does not use XML::Error.errors. The references to @@errors are only there to observe the state. And they show that error messages accumulate, without ever using the deprecated API.

Actually seems there's still a usage of it in the stdlib in arm context

The search results are from outdated copies of the standard library. That usage was removed in #13038.

💡 Idea: how about changing the backing storage of these errors to a single integer? Then the legacy method would return N times a fake error message

Sorry, I realized these both just before you replied 🙈. Thanks for the confirmation tho.

@oprypin That's an interesting thought. But I think it might be better to introduce a hard break and alert of the situation instead of putting the old API on some sort of minimal life-support.

Sure, personally I'm not against this breakage. The entrenched policy is clearly against it though 😬

I say it's better to change the behavior to just keep the last message, place that in the deprecation message, and be done with it. We keep some backward compatibility, but not the leak.

@oprypin No, I believe this is within the limits of the compatibility policy: https://crystal-lang.org/reference/1.13/project/release-policy.html#reservations

Bugs: if an API has undesired behaviour, a program that depends on the buggy behaviour may break if the bug is fixed. We reserve the right to fix such bugs.

XML::Error.errors cannot operate in a meaningful way without leaking memory. Fixing the memory leak requires breaking the method.

We can chose in which way to do that.

1. Remove it entirely. Any program using it will fail to build and thus alert about the issue.
2. Reduce storage to only the number of errors. Programs will continue to build. There won't be informative error messages. But it preserves the behaviour that the result is Nil if no error occured since the last access, and otherwise it's an Array(String) whose size tells the number of errors since last access.

I suppose another option could be to generalize the error messages (i.e. remove quotes of the input document such as element names) and put them in a string pool. That could restrict overall memory usage to a tolerable amount. But it requires a bit of effort just to keep supporting a deprecated API that is not very useful anyway.

Or as mentioned in the OP, we could put a cap on the number of stored error messages and drop old ones. That's similar to #14934 (comment) which puts the cap at 1.

After further considerations after #14966 's comments, I'm now convinced that this is a breakage we need to do. It's impact in the ecosystem should be very limited, and the attempts at having a backwards-compatible version that doesn't leak and still works is challenging at best. So I'm now in favor of simply blocking its usage. #14934 is nice in that it doesn't remove the methods, but instead shows a nice explanatory message of why using it is not allowed.