Skip to content

Final overloads + default/override (fix issue #383) #464

Closed
wants to merge 7 commits into from

2 participants

@ghost
ghost commented Jan 1, 2013

Implement final by default.
Add default & override keywords for overload refinement.
default overloads can be masked by subsequent overloads & overrides.
override overloads mask previous definitions (equivalent to previous overload behaviour).
Add some tests.

Notes:

Searching for ambiguous overloads exposed an issue with builtin constructors. If a field type is an expression to be evaluated using pattern cells the default constructor will fail. A workaround is to use computed record which doesn't generate a constructor overload.

Updated test/lang/uncategorized/45 to use a computed record definition to prevent generation of a faulty built-in constructer.

agemogolk added some commits Dec 14, 2012
agemogolk Make overloads final by default.
Introduce 'override' keyword to preserve previous overload behaviour.
Introduce 'default' overloads which can be overridden by either overloads or overrides.
Library uses override for now.

Add final tests.
7c81ffc
agemogolk Merge remote branch 'jckarter/master' into final2 bdac017
agemogolk Include override & default in claydoc d03510b
@jckarter
Owner
jckarter commented Jan 5, 2013

Sounds good, but the rename of overload to override seems a bit churny to me for little benefit. Can the issue with builtin constructors be detected by the compiler, so it can raise an error if you try to use the builtin constructor when it's not valid?

@ghost
ghost commented Jan 5, 2013

The overload/override keywords are a bit naff. I have experimented with removing overload (# 457), however the benefits are marginal due to syntactic inconsitencies e.g. standalone definitions, interfaces, overloads. Maybe standalone could be removed, it does seem somewhat redundant with default final and the private overload constraint.

I'm not entirely happy with the default overload. Currently it is available anywhere in the overload sequence. Maybe it would be better to tether a default definition to a define in some way.

It should be possible to detect a faulty generated constructor. A flag on the overload or some thing more sophisticated, I'll open a new ticket and take a look at it.

@ghost
ghost commented Jan 5, 2013

To be clear, the overload keyword is still in use here.

overload is the standard way to overload a symbol, however it can only mask a default e.g.

define foo;
default foo(T) = false;
overload foo(#Int32) = true; //Valid override of foo
overload foo(#Int64) = true; //Valid override of foo

override masks any prior definition e.g.

define foo;
overload foo(a:Int32, b:Float64) = ...;
overload foo(a, b) = ...;  //Invalid as overload cannot override

override foo(a, b) = ...; //Valid overriding of previous foo(a:T, b:U) overloads

I've changed all overloads in the library/compiler to be override's as this preserves the existing functionality. All modules will need to be reviewed to take advantage of default final.

@jckarter
Owner
jckarter commented Jan 5, 2013

I see; that makes sense. Sounds good to me.

agemogolk added some commits Jan 6, 2013
agemogolk Merge remote branch 'jckarter/master' into final2 95f8e2a
agemogolk Update compiler overload status flags.
Fix libs that use records with properties. Provide default overloads.
Only check for ambiuous matches when match isn't an override.
3c5540a
agemogolk Re-enable dodgy tests ec37a53
agemogolk Merge branch 'master' into final2 da46828
@ghost
ghost commented Jan 11, 2013

@jckarter @stepancheg @galchinsky Any objections to merging this prior to library overhaul? Any issues I may have missed?

@stepancheg
Collaborator

@agemogolk I (as usually) would like to complain about quality of commits.

  • 3 of 7 commits are "merge remote branch" they are really not necessary in history. History should be squashed into one or two commits, each of which denotes complete piece of work
  • some code uses tab for indentation, while the most project code uses 4 spaces indentation
  • test names (1, 2, 3 and 4) are meaningless, should be something like (overload-overload-valid, overload-overload-invalid, overload-default-valid and overide-overload-invalid)
  • some test scenarios are missing. For instance, can I override another override? Can I overload default overload with another default overload?

And the worst thing it is really hard to understand exact semantic of default/overload/override keywords from your description. Compare to description of my original proposal: here.

Good description should contain strict description of algorithm, and should be illustrated by code examples (tests are not enough). And such description should be included in commit message for people who read commit history. This description can be later copied into the language specification.

Bottom line: I like it, but I'm afraid I missed something important.

@ghost
ghost commented Jan 11, 2013

@stepancheg constructive criticism is always welcome.

Sorry the explanation of semantics is not clear to you. In answer to your queries:

  • override can override anything (hence the name).
  • default is an overload that can be overriden by either overload or override.
  • default cannot override anything ensuring that there is only one default overload. Also, a default cannot be set once an overload/override has already been defined.
@stepancheg
Collaborator

@agemogolk Well, I think override that can override anything conflicts with basic idea of "final overloads": final overloads should not be overloadable. This is the point of final overloads: if you see invocation foo(1), and you see final overload foo(param), you can be sure that no other part of project changes behavior of foo(1). Truly final overloads make code easier to read and maintain.

I prefer previous version of this feature. Repeating myself:

  • standalone functions cannot be overloaded even if parameters do not collide
  • default overloads can be re-overloaded
  • non-default overloads cannot be re-overloaded

Syntax:

foo(x) = 1; // standalone
overload foo(x, y, z) = 2; // ERROR: any overload of foo is invalid

define bar;
default bar(x) = 1; // default overload
default bar(x) = 2; // it is OK to override default overload with another default overload
overload bar(x: Int) = 3; // final overload for Int parameter
[S when String?(S)]
overload bar(x: S) = 4; // final overload for String? parameter

overload bar(s: CStringRef) = 5; // ERROR: because overload for String? is final
@jckarter
Owner

I'm with @stepancheg that it'd be ideal to only have 'default' and 'non-default' overloads, which (if I understand correctly) correspond to default and overload in this scheme. However, override seems like a good migration shim that preserves the old behavior while the library gets overhauled. If override can be completely replaced by default or the new overload in the library, then it'd make sense to drop it once the library's been migrated. That sound good to both of you?

@stepancheg
Collaborator

@jckarter

I'm not sure that temporary override keyword won't become permanent.

There's easier migration path.

1.

  • Implement overload and default with proposed behavior (overload is final)
  • Add temporary compiler option "-overload-is-final", that is off by default
  • When "overload-is-final" option is off, violations of final overloads are ignored (except for standalone functions and private overloads, that are good already)
  • Commit the code. No library changes necessary.

2.

From time to time try to compile tests with -overload-is-final option on, fix library by replacing some overload with default

3.

When all tests become working with -overload-is-final option on, drop the option "-overload-is-final", and make violation of final overload unconditional error

@stepancheg
Collaborator

@agemogolk I forgot to mention, that I think that default overload should be able to override another default overload. Use case:

define Printable?(x): Bool;

default Printable?(x) = false;

[S when Sequence?(S)]
default Printable?(#S) = true; // overrides previous default

[A]
overload Printable?(#InfiniteSequence[A]) = false;

default and overload are a bit similar to virtual and (no-specifier-that-means-final) in C#: you may override virtual with another virtual or final, and you cannot override final.

@ghost
ghost commented Jan 12, 2013

I agree with @stepancheg's approach, temporary keywords should be avoided. @jckarter, to be clear, do you agree that default should be allowed to override another default.

@ghost
ghost commented Jan 12, 2013

@stepancheg I think there is some confusion going on, my original proposal was to have default be overridable by an other type of overload, apologies if that wasn't clear. All that needs to happen is removal of override.

@jckarter
Owner

I think that ideally you'd have it so that default overloads can't override each other either, so that there are most two overloads that can influence any call site. In practice I don't know if that'll work out because of cases like @stepancheg's example, so I'm OK with defaults being able to override each other. Maybe an alternative sanity constraint would be to limit default overloads so that they can only appear in the same module as the symbol definition.

@ghost
ghost commented Jan 12, 2013

Constraining default to a single module sounds reasonable. I'll remove the override keyword, tidy-up and add the compiler flag as per @stepancheg suggestion and see how it looks.

@stepancheg
Collaborator

@jckarter @agemogolk I think default overloads limited to one module is too restrictive. In my example above (about Printable?), printable module (that defines Printable?) may not know about sequences, thus printable module cannot add second default overload.

@jckarter
Owner

@stepancheg You're right. You need to be able to refine concepts independent of the original module, so limiting defaults isn't going to work.

@ghost Unknown referenced this pull request Jan 13, 2013
Merged

Final overloads #478

@ghost
ghost commented Jan 13, 2013

See issue #478 for updated implementation.

@ghost ghost closed this Jan 13, 2013
@ghost ghost deleted the unknown repository branch Jan 13, 2013
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.