While building the apply-types tool into the compiler (for fun and no profit), I’ve gotten to the point where I can safely run it on the compiler itself and have it spit out correct looking type restrictions. However, the tool doesn’t know if a given method is overriding a parent method declaration, and so what gets output no longer compiles due to abstract defs no longer being overriden correctly. I can find and detect these cases by brute force checking the ancestors for each type and method (we’ll ignore performance issues for now), but was thinking that a developer explicitly stating whether a method overrides another method or not would be really helpful for devs and tools alike.
I discovered this initial request from 9 years ago essentially introducing the exact idea I was thinking:
And it looked like an initial PR was setup by Asterite to introduce @[Override] and a @[Redefine] annotations, but was then closed due to needing to go through a “design process”:
I’m curious if others would still find this type of annotation / behavior (as a purely optional protection, similar to type restrictions in general) beneficial to the language, and if so, what would the starting point for a design process be today? Start an RFC?
I personally prefer more verboseness for long-term code / library code than less, so being able to enforce that subclass methods that override use a specific annotation (and otherwise produce a lint warning or an error) is a bonus. Others will probably disagree though (which is fine), the language is already complex enough as-is. It may be difficult to enforce something like this now.
An RFC would be the proper channel to go through for creating something like this, though with my recent RFC I started with a forum post (just like this) to garner ideas and feedback before putting an RFC together.
Same regarding more verboseness for long-term projects. At some point a future me comes through the code and has completely forgotten what it’s purpose is, so having some more explicitly defined parts are really helpful for that. I also don’t want to lose the magic of crystal, so for this @[Override] annotation I’d treat it in the same vein as type restrictions in general: optional, but if specified, can help protect your code against mistakes.
To make it explicit, here are my own preferences to what the behavior would be:
If no @[Override] annotation is present, then everything behaves the same as today (no change, overriden methods are inferred based on method name and argument types).
If an @[Override] annotation is present, then whatever def it hangs on, must override at least one ancestral method. If it does not, this gets treated as a compiler error.
In the initial thread, the main use case was for typos in the method name, but the use case I’m running into is changing the type restrictions to something more strict but still different from the parent def now no longer matches either.
If anyone has any other thoughts on this annotation’s behavior, or agree or disagrees with my own thoughts, please chime in!
TypeScript has a feature related to this via the override keyword (versus annotation), and a compiler flag that makes it an error to override any method from a superclass unless you explicitly use an override keyword.
IMO this is a linting concern, it doesn’t have much, if anything, to do with the compiler. Save for the annotation type(s), this can be implemented as an Ameba rule. Maybe it wouldn’t even need annotations: typos can be detected with a levenshtein distance, same name but different signature, etc.
You like it? You enable the rule, you don’t like it, you disable the rule. This only applies to your projects and libraries, and your personal preference won’t leak to users of your library: they can enable/disable the rule or not use a linter: it’s their choice.
This would be a static analysis concern, linting would be too superficial to catch it. Since the compiler owns the semantic processing, and the exclusive decision making in which method overrides another method (as determined on def name matching and inferred argument type matching heuristics), this at the very least requires the full semantic processing. Since the compiler is, like, %60 semantic processing (I’m making this number up, but it’s a non-trivial fraction), I’d say it makes sense that it has something to do with the compiler.
While typos have been the main use case, the other problems I want to address are:
As a new developer entering a code base, I see a method I’m not familiar with not having any type restrictions on it, and I add them incorrectly. Since the original method is still around, the compiler sees nothing wrong, compiles, and now I have a runtime bug using the incorrect parent implementation. (This is where the apply-types tool exposed me to the problem)
As an author of a popular base library, I made an update to what I think is an internal method, either changing the name or the arguments. I do not realize that other libraries / applications have overridden it for some particular reason or another, and when they update their shards, they now have a runtime bug.
(for completeness) As dev quickly iterating on things, I try to override the parent method foo but accidentally type in fooo instead. No issues are raised up and I now have a runtime bug.
All runtime bugs are conceivably caught by tests, but that’s always additional work and thoughtfulness, and not a guarantee anyways. Interestingly, the opposite direction isn’t a problem - I can mark a method as abstract and force all subclasses to implement it or else it’s a compiler error. This, too, came up during the apply-types testing, but the compiler caught these errors for a different reason.
Unless I’m missing something, the proposal still respects this - libraries using this annotation would only have the rule applied to their code, at the site of the annotation while technically other libs / applications compiling with these annotated lib methods would trigger the same check, it should presumably always pass, since the overridden method should be available. Or I missed the point of this paragraph
PHP has recently introduced such an annotation, and I hated it on sight.
I do get the arguments about protecting against typos and superclasses getting a new method that matches one in your subclass, but if we decide that we want that, why not make it part of the language (I’ll confess to not reading the linked issues)?
I can see some value in making redefinition explicit, but make it short and sweet, redef as it’s done all the time. And contrary to type restrictions where you can be sorta “I’ll figure out the exact pitch of quack later”, you really shouldn’t be in doubt whether you’re overriding or not.
redef was brought up several times in the linked issue and seemed like a popular option. The only opposition I saw was from this post, which points out we already have syntax for special types of defs via abstract def and private def, and so proposed override def to remain consistent with those.
I made this post proposing this as an annotation since that fit into the already existing language syntax and represents a smaller change (and smaller risk to the language), but I also like the idea of really making it part of the language too.
Well, yeah… But you can’t really redefine a private method, can you? Doesn’t it just stay private in the superclass, and you get your own private version? And redeffing an abstract method is kinda the point. I’m not convinced, private and abstract are kinda properties of the following def, while redefinition is a bit more than just a property, in my mind.
But it needs a bit of thinking. If you reopen a class and redefine a method that way, do you use redef? You’re redefining, but not in an OO way.
private doesn’t work like it does in PHP. Crystal’s private is more akin to PHP’s protected modifier, in that child classes can access private methods defined in the parent, but you can’t use them outside of the class itself. So because of this it would be possible for a child to re-define a private method defined on the parent and if the parent used that private method, the child’s implementation would be used instead.
while technically other libs / applications compiling with these annotated lib methods would trigger the same check, it should presumably always pass, since the overridden method should be available. Or I missed the point of this paragraph 