Skip to content

Conversation

@edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Oct 17, 2023

Fixes #8547

  • Normal let bindings
  • Recursive let bindings
  • More testing
  • LangVersion flag

@vzarytovskii
Copy link
Member

LangVersion flag ?

Yes, at the very least. However, just language version will not be good enought for people who will all of the sudden have this issue when upgrading SDKs.

I.e. it won't explain why do the have it now and how do they rollback to previous behaviour.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Oct 17, 2023

LangVersion flag ?

Yes, at the very least. However, just language version will not be good enought for people who will all of the sudden have this issue when upgrading SDKs.

I.e. it won't explain why do the have it now and how do they rollback to previous behaviour.

Hum, a new error number and message it is the way to go here

@edgarfgp

This comment was marked as outdated.

@edgarfgp
Copy link
Contributor Author

Findings:

  • The __useResumableCode is a let-bound value but has the Attribute MethodImpl(MethodImplOptions.NoInlining) which should only be used with (AttributeTargets.Method | AttributeTargets.Constructor)
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)]
    sealed public class MethodImplAttribute: Attribute
    {
[<MethodImpl(MethodImplOptions.NoInlining)>]
let __useResumableCode<'T> : bool = false

Questions:

  • Should enforce the AttributeTargets.Method to __useResumableCode and update the resumable.fs
  • Make an exception for `MethodImpl(MethodImplOptions.NoInlining) to be used in let values.

cc @vzarytovskii

@vzarytovskii
Copy link
Member

MethodImpl is something we don't control, right? I don't think we should hack attribute checking in compiler.

And __useResumableCode is not a value, it's a type function and compiler intrinsic at the same time.

What codegen does it currently result in?

@vzarytovskii
Copy link
Member

So, I think whatever rules we enforce now for functions (AttributeTargets.Method, I suppose?), should also be enforced for type functions.

@edgarfgp
Copy link
Contributor Author

And __useResumableCode is not a value, it's a type function and compiler intrinsic at the same time.

This makes more sense now . Thanks

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 29, 2023

[<AttributeUsage(AttributeTargets.Method,AllowMultiple=false)>]
[<Sealed>]
type TailCallAttribute() =
     inherit System.Attribute()

[<TailCall>] 
let someX = 23

With this PR will also enforce that you can not use [<TailCall>] cc @dawedawe


[<Fact>]
let ShouldTriggerCompletionAtStartOfFileWithInsertion =
let ShouldTriggerCompletionAtStartOfFileWithInsertion () =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better now :)

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

F# compiler does not enforce attribute targets

2 participants