Prevent accidental proc overwriting, proc signature overloads

[LadyCailin]

A discussion came up about being able to overwrite procs. There are definitely good use cases for being able to overwrite procs, but the discussion was that perhaps this is not a good thing by default/generally.

A number of proposals were offered, summarized here. For clarity, the term "final" is taken to mean that a proc cannot be overwritten, whether or not there is a keyword final involved.

1. No change in existing behavior, but a new keyword final is introduced, and when applied to procs, prevents them from being overwritten elsewhere.

• Pros: No user code has to change. The implementation is simpler, and doesn't require a phase in period.
• Cons: If a user is accidentally overwriting a proc when they didn't mean to, this feature will not benefit them.

2. Add overwrites/overwritable/overwrite keyword(s). This changes existing behavior, so that all procs are final by default, but procs that are marked with some combination of either/both overwrites and/or overwritable are allowed to be overwritten.

• Pros: It's impossible for users to miss accidental overwrites, as this is much stricter.
• Cons: Behavior change, requires users to change code and requires a phase in period. It seems unlikely that this would be a widespread problem, but in general this would require a phase in period, and we should create telemetry on this to count how many people are affected, and the magnitude that they are affected before making this functionality visible to users at all.

For option 2, there is still a need to determine the specific keywords that are used. Do you put the overwritable keyword on the "master" proc, and the overwrites keyword on the sub-proc? Or do you just have a single keyword overwrite which you tag on all procs that are intended on being overwritten and the procs that overwrite them? This is a minor discussion that needs to happen still, but does not materially change the overall decision.

Another discussion is to also perform signature matching on non-final procs. So redefining a proc (which is either implicitly or explicitly overwritable) with a new signature should be disallowed. I generally support this, but this is a different decision and implementation. This may however prevent valid use case, so further user research needs to be done to provide input to this decision.

[VergilPrime]

I'm not even sure what the current behavior is, so I'm not sure what would be best :)

[LadyCailin]

Current behavior is that the last definition takes precedent.

proc _a() {
        msg("First definition");
}

proc _a() {
        msg("Second definition");
}

_a(); // Second Definition

[VergilPrime]

Well currently there's no way of knowing what files run first, so having a final tag could potentially have use but...

Because of the way it currently works, I don't think anyone would be careless enough to define the same proc twice, I know I sure wouldn't. That uncertainty currently keeps us in kind of a rigid structure.

Consider how Java and C# work where you not only have to match the name of a method but the number and type of variables in order to trigger a specific override of a method. MethodScript doesn't have that so if a proc is updated to require more/fewer/different arguments, you have to go change that throughout your code. Final won't solve that problem because instead of selectively updating different parts of code that use it, it just forces one version to essentially override any other version.

Maybe instead of final you could implement something like overloads or a version system, something like ver=1.3 which would be used unless a higher version of the proc is found elsewhere. That way at least if we were able to distribute zipped packages it would be easier, as we could just include all our procs with each package and still ensure that whomever is using them has the latest version.

What real use case is there for final?

[PseudoKnight]

The lack of file order only matters for auto_includes. There is never a use for proc overriding there, so it can always be detected and have a warning. (ie. all procs in auto_includes should be treated as final)

[VergilPrime]

Again, what real use case is there for final?

[LadyCailin]

The same use case as the final keyword in java, for instance. For cases where the method isn't meant to be overridden. In general, the purpose is to be able to prevent overriding where it doesn't make sense from a business logic perspective. Take for instance a utility method, which simply adds two numbers together, called _add. There's no case where it makes sense to have another method somewhere else named _add that does something different (or even the same thing, just defined twice), so if you've inadvertently created a second _add method, then it's wrong. In either proposal, adding the ability to make procs final can help prevent accidental duplication like this.

[VergilPrime]

If procs in auto_includes are non-supported and throw a warning, then there's no reason why two _add procs should ever be a problem. Three possibilities:

1. Two _add procs are in the same file. This is obviously a mistake, just warn the user.

2. Two _add procs are in different files, assume they are different packages. It doesn't matter at all because you would only include() the one you want anyways. If you have to include() both then it's just as easy to rename one proc as it is to add a final keyword.

3. Two _add procs are in auto_includes. You're changing the behavior here but it's not practically useful, if you chose to use the same name twice, and want one to always override the other, why on earth did you use the same name twice? Like Pseudo said, there's never a reasonable use for proc overriding here.

Final keywords in Java/C# exist because overriding is something you might actually want to do. Say you install a package you don't want to mess with, but you want to add properties or override a particular method. One good example of this is importing Microsoft's Identity packages which handle user logins and OAuth. It comes with a class for a user, but maybe you want to add a property for the user's associated Minecraft account or something. You can override the constructor to get that information on account creation for instance.

The reason that is useful is because classes are big huge things with many parts. Procs are just code snippets, it's pretty much all or nothing in terms of modifying. No reason to intentionally override a proc means no reason to lock it down either.

[LadyCailin]

1. Not necessarily, there might be a default implementation, and a conditional redefine, consider this:

proc _a() {
    // default
}

if(@asdf) {
    proc _a() {
        // new behavior
    }
}

Of course, in this case, this is intentional, but in the second proposal, this currently valid code would require changes (albeit simple ones).
2. You make some assumptions here that I don't think you can make. Depending on how one's code is organized, you might in fact include both files.
3. My comment about #1 applies here too actually.

But in any case, it seems like you're arguing against both proposals, rather than for one or the other. In both proposals, there is now a way to have final procs, it's just whether that's the default or not, and which keywords are used to make the procs be final.

[PseudoKnight]

To be clear, I use proc overriding in a few projects. It is useful. It is current behavior. (there could be instances in people's scripts where a file is included twice) There's just no good reason to override an auto_included proc that I can think of. It probably shouldn't be in an auto_include if one were overriding it.

[LadyCailin]

In my example #1, I think that's a compelling use case, but that's within the same file. But you could just as easily break that into a separate file with no issues.

Also, the functionality wouldn't be removed, that option isn't on the table at all. The question is whether or not to make it so that it's the default or requires a keyword.

[VergilPrime]

Also couldn't...

proc _a() { if(@asdf) { // new behavior }else{ //default behavior } }

[LadyCailin]

Not necessarily. Consider the case where it logically makes sense to define it in different files, perhaps if you're replicating like a stubbing system.