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

Warn on implicit type functions, or an attribute to amortize them using type-indexed table [ RFC FS-1038 ] #602

Open
jack-pappas opened this Issue Aug 12, 2017 · 18 comments

Comments

Projects
None yet
5 participants
@jack-pappas

jack-pappas commented Aug 12, 2017

I propose a modification to the way the compiler emits code for module-scoped, let-bound values decorated with [<GeneralizableValue>].

When defining a type-indexed, generalizable value, like so:

module MyModule =
    [<GeneralizableValue>]
    let foo<'T> = typeof<'T>.Name

the compiler changes the compiled representation of the value from a static field into a static method. This means the value is really changed to a function, and the expression bound to the value is re-evaluated each time the value is used. In other words, the compiler produces code that'd decompile into something like the following C# code:

static class MyModule
{
    public static string foo<T>()
    {
        return typeof(T).Name;
    }
}

If the compiler generated a slightly more-complex backing representation for each 'generalized' value, we could get the benefits of type-indexed values without having to re-evaluate the expression each time the value was used. My suggestion is that, for each such field, the compiler generates a nested, private, static, generic, beforefieldinit class containing a private, static, readonly field and a public get-only property to fetch the field's value. The expression (rhs) being bound to the generalizable value would be compiled into this class' static constructor (.cctor) and used to initialize the readonly field within the class. The compiler would still also emit a static method for the generalizable value, but instead of re-evaluating the expression, the method would simply call the property getter to fetch the evaluated, cached value from within it's corresponding static class. To illustrate, the code emitted for the value would decompile to something like the following C# code:

static class MyModule
{
    [CompilerGenerated]
    private static class foo__BackingField<T>
    {
        private static readonly string __value = typeof(T).Name;

        public static string Value { get { return __value; } }
    }

    public static string foo<T>()
    {
        return foo__BackingField<T>.Value;
    }
}

Pros and Cons

The advantages of making this adjustment to F# are:

  • Expressions bound to let-bound values decorated with [<GeneralizableValue>] are evaluated only once, just like any other let-bound value would be.
  • May provide a small performance improvement for cases where it's expensive (for one reason or another) to evaluate the expression being bound to the value.

The disadvantages of making this adjustment to F# are:

  • Code which currently relies on the multiple-evaluation aspect of [<GeneralizableValue>] would break.
  • Could increase memory usage and/or change GC characteristics due to actually caching evaluated values; they'll be stored in static fields, so if the values are instances of reference types they'll become GC roots.
  • Small increases in compiled binary sizes due to the additional metadata for the generated backing classes.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [✓] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [✓] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [✓] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • [✓] This is not a breaking change to the F# language design
  • [✓] I or my company would be willing to help implement and/or test this
@jack-pappas

This comment has been minimized.

Show comment
Hide comment
@jack-pappas

jack-pappas Aug 12, 2017

For additional background information, see this article "Finer Points of F# Value Restriction":
https://blogs.msdn.microsoft.com/mulambda/2010/05/01/finer-points-of-f-value-restriction/

jack-pappas commented Aug 12, 2017

For additional background information, see this article "Finer Points of F# Value Restriction":
https://blogs.msdn.microsoft.com/mulambda/2010/05/01/finer-points-of-f-value-restriction/

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 14, 2017

Collaborator

@jack-pappas I like this suggestion. I suppose it is in theory a breaking change if the evaluation has an observable side-effect. But GeneralizableValue is extremely rarely used, and the intent is that it is only used in situations where amortization is reasonable, so I don't think that's actually a problem in practice

Collaborator

dsyme commented Aug 14, 2017

@jack-pappas I like this suggestion. I suppose it is in theory a breaking change if the evaluation has an observable side-effect. But GeneralizableValue is extremely rarely used, and the intent is that it is only used in situations where amortization is reasonable, so I don't think that's actually a problem in practice

@jack-pappas

This comment has been minimized.

Show comment
Hide comment
@jack-pappas

jack-pappas Aug 14, 2017

Thanks! Should I write up an RFC for this?

jack-pappas commented Aug 14, 2017

Thanks! Should I write up an RFC for this?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 14, 2017

Collaborator

Yes, marking as approved

Collaborator

dsyme commented Aug 14, 2017

Yes, marking as approved

@jack-pappas

This comment has been minimized.

Show comment
Hide comment
@jack-pappas

jack-pappas Aug 27, 2017

I've submitted an RFC (FS-1038) to fslang-design so we can start working out the details.

jack-pappas commented Aug 27, 2017

I've submitted an RFC (FS-1038) to fslang-design so we can start working out the details.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 29, 2017

Collaborator

@jack-pappas I'll leave the suggestion open so people can find it

Collaborator

dsyme commented Aug 29, 2017

@jack-pappas I'll leave the suggestion open so people can find it

@dsyme dsyme reopened this Aug 29, 2017

@eiriktsarpalis

This comment has been minimized.

Show comment
Hide comment
@eiriktsarpalis

eiriktsarpalis Aug 29, 2017

@dsyme @jack-pappas the proposed change would irrevocably break certain applications where it is required that a fresh object be returned when invoked for reference types. Check this example for instance.

eiriktsarpalis commented Aug 29, 2017

@dsyme @jack-pappas the proposed change would irrevocably break certain applications where it is required that a fresh object be returned when invoked for reference types. Check this example for instance.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 29, 2017

Collaborator

@eiriktsarpalis Interesting, thanks.
@jack-pappas Using opt-in for the generation of a type-specific backing field would be a way forward with this:

    [<GeneralizableValue(unique=true)>]
    let foo<'T> = typeof<'T>.Name
Collaborator

dsyme commented Aug 29, 2017

@eiriktsarpalis Interesting, thanks.
@jack-pappas Using opt-in for the generation of a type-specific backing field would be a way forward with this:

    [<GeneralizableValue(unique=true)>]
    let foo<'T> = typeof<'T>.Name
@eiriktsarpalis

This comment has been minimized.

Show comment
Hide comment
@eiriktsarpalis

eiriktsarpalis Aug 29, 2017

@dsyme @jack-pappas would it make sense to include the GeneralizableValue attribute in code generation as well? It would solve one of the issues described here.

eiriktsarpalis commented Aug 29, 2017

@dsyme @jack-pappas would it make sense to include the GeneralizableValue attribute in code generation as well? It would solve one of the issues described here.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 29, 2017

Collaborator

@eiriktsarpalis Yes. Though do you also need it for generalized values without the attribute like

let x = [[]]

?

Collaborator

dsyme commented Aug 29, 2017

@eiriktsarpalis Yes. Though do you also need it for generalized values without the attribute like

let x = [[]]

?

@eiriktsarpalis

This comment has been minimized.

Show comment
Hide comment
@eiriktsarpalis

eiriktsarpalis Aug 29, 2017

Yes, anything of that sort.

eiriktsarpalis commented Aug 29, 2017

Yes, anything of that sort.

@jack-pappas

This comment has been minimized.

Show comment
Hide comment
@jack-pappas

jack-pappas Sep 10, 2017

According to the blog post I linked above:

If you end up writing a type-parametrized function returning mutable objects or having side effects, please do sprinkle a RequiresExplicitTypeArguments attribute over it:

[<RequiresExplicitTypeArguments>]
let v<'T> : 'T list ref = ref []

Sounds that's what should be used instead of adding a Unique property to [<GeneralizableValue>]?

jack-pappas commented Sep 10, 2017

According to the blog post I linked above:

If you end up writing a type-parametrized function returning mutable objects or having side effects, please do sprinkle a RequiresExplicitTypeArguments attribute over it:

[<RequiresExplicitTypeArguments>]
let v<'T> : 'T list ref = ref []

Sounds that's what should be used instead of adding a Unique property to [<GeneralizableValue>]?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 10, 2017

Collaborator

@jack-pappas RequiresExplicitTypeArguments is a very syntactic thing. I think adding the flag to GeneralizableValue is right

Collaborator

dsyme commented Sep 10, 2017

@jack-pappas RequiresExplicitTypeArguments is a very syntactic thing. I think adding the flag to GeneralizableValue is right

@abelbraaksma

This comment has been minimized.

Show comment
Hide comment
@abelbraaksma

abelbraaksma Sep 19, 2017

Using opt-in for the generation of a type-specific backing field would be a way forward with this:

@dsyme, you suggest unique = true, but that may be misinterpreted, as it may also mean: getting a unique (reference) value each time the value is used, as in @eiriktsarpalis's example.

perhaps the following would be more self-explanatory (or even better, perhaps, an enum, which is easier to expand later if need arises and it helps semantically, as it prevents people from writing GeneralizeableValue(true), which in and of itself appears to apply to it being generalizable or not).

[<GeneralizableValue(initializeOnce=true)>]
let foo<'T> = typeof<'T>.Name

abelbraaksma commented Sep 19, 2017

Using opt-in for the generation of a type-specific backing field would be a way forward with this:

@dsyme, you suggest unique = true, but that may be misinterpreted, as it may also mean: getting a unique (reference) value each time the value is used, as in @eiriktsarpalis's example.

perhaps the following would be more self-explanatory (or even better, perhaps, an enum, which is easier to expand later if need arises and it helps semantically, as it prevents people from writing GeneralizeableValue(true), which in and of itself appears to apply to it being generalizable or not).

[<GeneralizableValue(initializeOnce=true)>]
let foo<'T> = typeof<'T>.Name
@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 20, 2017

Collaborator

@abelbraaksma Yes, that would be fine. Or amortized or once is fine too, other suggestions welcome

Collaborator

dsyme commented Sep 20, 2017

@abelbraaksma Yes, that would be fine. Or amortized or once is fine too, other suggestions welcome

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Sep 20, 2017

It would be really unfortunate if we cannot fix this.
It doesn't look like a function and additionally the attribute says it's a "value".

Was it clearly speced to behave like this or was the implementation just an oversight (= read bug)?

matthid commented Sep 20, 2017

It would be really unfortunate if we cannot fix this.
It doesn't look like a function and additionally the attribute says it's a "value".

Was it clearly speced to behave like this or was the implementation just an oversight (= read bug)?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 16, 2017

Collaborator

Was it clearly speced to behave like this or was the implementation just an oversight (= read bug)?

It was intended - I'm not saying the name was good though.

The idea was "the library designer intends to implement something that feels like a value to the user of the library (like Set.empty) and thus needs it to be generalizable"

Note that it is not GeneralizableValue that actually causes the use of a type function. The effect of GeneralizableValue is to make uses of a thing generalizable, e.g. c.f.

type Set<'T> = 'T list

let zero : Set<Set<'T>> = [ [] ] // no value restriction - implicit type function for ML-style generalizable "value"

c.f.

type Set<'T>() = class end

let empty<'T> = Set<'T>()

let zero = [ empty ] // value restriction

c.f.

type Set<'T>() = class end

[<GeneralizableValue>]
let empty<'T> = Set<'T>()

let zero = [ empty ] // no value restriction, implicit type function

As a result I don't think adjusting the GeneralizableValue attribute is really what you're after - a new TypeIndexedValue attribute may be better

I could also see the case for an optional warning whenever a implicit type function is defined via generalization of a non-function value, e.g. let empty = [ [ ] ].

Collaborator

dsyme commented Nov 16, 2017

Was it clearly speced to behave like this or was the implementation just an oversight (= read bug)?

It was intended - I'm not saying the name was good though.

The idea was "the library designer intends to implement something that feels like a value to the user of the library (like Set.empty) and thus needs it to be generalizable"

Note that it is not GeneralizableValue that actually causes the use of a type function. The effect of GeneralizableValue is to make uses of a thing generalizable, e.g. c.f.

type Set<'T> = 'T list

let zero : Set<Set<'T>> = [ [] ] // no value restriction - implicit type function for ML-style generalizable "value"

c.f.

type Set<'T>() = class end

let empty<'T> = Set<'T>()

let zero = [ empty ] // value restriction

c.f.

type Set<'T>() = class end

[<GeneralizableValue>]
let empty<'T> = Set<'T>()

let zero = [ empty ] // no value restriction, implicit type function

As a result I don't think adjusting the GeneralizableValue attribute is really what you're after - a new TypeIndexedValue attribute may be better

I could also see the case for an optional warning whenever a implicit type function is defined via generalization of a non-function value, e.g. let empty = [ [ ] ].

@dsyme dsyme changed the title from Evaluate 'generalizable' values once, like other let-bound values to Option to warn on implicit type functions, or an attribute to amortize them using type-indexed table Nov 16, 2017

@dsyme dsyme changed the title from Option to warn on implicit type functions, or an attribute to amortize them using type-indexed table to Warn on implicit type functions, or an attribute to amortize them using type-indexed table Nov 16, 2017

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 16, 2017

Collaborator

@jack-pappas I've adjusted the title of the suggestion - RFC will need to be updated too?

Collaborator

dsyme commented Nov 16, 2017

@jack-pappas I've adjusted the title of the suggestion - RFC will need to be updated too?

@dsyme dsyme added needs rfc and removed needs rfc labels Dec 1, 2017

@dsyme dsyme changed the title from Warn on implicit type functions, or an attribute to amortize them using type-indexed table to Warn on implicit type functions, or an attribute to amortize them using type-indexed table [ RFC FS-1038 ] Dec 1, 2017

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