Skip to content
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

Allow usage of [<TailCall>] with older FSharp.Core package versions #16373

Merged
merged 12 commits into from
Dec 13, 2023

Conversation

dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Dec 4, 2023

Description

This allows users to use the [<TailCall>] analysis with older versions of FSharp.Core. They just need to define the attribute in Microsoft.FSharp.Core themselves.

Checklist

  • Test cases added
  • Performance benchmarks added in case of performance changes
  • Release notes entry updated

@dawedawe dawedawe requested a review from a team as a code owner December 4, 2023 10:26
@vzarytovskii
Copy link
Member

Just a note - while it's a valid scenario, we highly discourage people from using custom FSharp.Core.

- move hasTailCallAttrib from TailCallChecks.fs to TcGlobals
@smoothdeveloper
Copy link
Contributor

What I like here is, like in C# which has those attributes, they can be added as internal in older projects that don't reference the recent SDK, and the compiler just look at those the same as if they were baked in the SDK. (for C#, I'd prefer record and nullable support to be shipped to .net framework consumers though...).

I don't think it is about "custom version of FSharp.Core" but just playing nice with consumers that can't easily (or just haven't done the) upgrade to latest one, if this comes at a very minor cost.

Taking the opposite approach to extreme (not saying this is what is done, F# is overall better than wider .net on this front, IME), even if it is "2 years were given", I see it a bit the same as "python 2 to 3", but this is so long it is very minor cost in the compiler impl.

If something is possible for string interpolation with FSharp.Core < 5, it may be worth a look, and to keep in mind for other attributes used by the compiler.

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 4, 2023

What I like here is, like in C# which has those attributes, they can be added as internal in older projects that don't reference the recent SDK, and the compiler just look at those the same as if they were baked in the SDK. (for C#, I'd prefer record and nullable support to be shipped to .net framework consumers though...).

We do the same for implicit attributes. Can't do with explicit ones.

I don't think it is about "custom version of FSharp.Core" but just playing nice with consumers that can't easily (or just haven't done the) upgrade to latest one, if this comes at a very minor cost.

The only situation where it's needed is when newer compiler is using older fslib. And the only way of doing is to upgrade sdk but specifically pin core lib to old one. And that's generally discouraged.

@smoothdeveloper
Copy link
Contributor

The only situation where it's needed is when newer compiler is using older fslib. And the only way of doing is to upgrade sdk but specifically pin core lib to old one. And that's generally discouraged.

I may misunderstand, but I think the encouraged thing is:

  • library pin to lower FSharp.Core, per what they need (not forcing consumers to update due to a dependency)
  • applications / leaf projects that are close to make runtime decision have the latitude to pick any FSharp.Core version, so long the compiler supports it

We encourage to support old for "inner / dependencies", or new for "close to runtime".

In this context, one library may decide to remain on lower FSharp.Core but define couple of internal attributes for their own consumption of features of recent F# compiler; it would generally not affect consumers of the library that may still use FSharp.Core that doesn't or does have those attributes.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Hmm I am also not a fan of the string comparison but since I don't have a better idea and this seems to be quite an isolated piece of functionality (and also edge-cased for the older F# core), I am fine with letting this in.

…ribute.fs

Co-authored-by: Petr <psfinaki@users.noreply.github.com>
@vzarytovskii vzarytovskii enabled auto-merge (squash) December 13, 2023 17:31
@vzarytovskii vzarytovskii merged commit e77d53b into dotnet:main Dec 13, 2023
26 checks passed
OwnageIsMagic added a commit to OwnageIsMagic/fsharp that referenced this pull request Dec 21, 2023
* upstream/main: (166 commits)
  typo in foldBack summary (dotnet#16453)
  Fix for dotnet#83 (improve constraint error message) (dotnet#16304)
  Name resolution: resolve interfaces in expressions (dotnet#15660)
  AddExplicitReturnType refactoring (dotnet#16077)
  Disabling 2 tests: running for too long, causing CI timeouts
  Improve value restriction error message dotnet#1103 (dotnet#15877)
  Parens: Keep parens for non-identical infix operator pairs with same precedence (dotnet#16372)
  More release note entries (dotnet#16438)
  Using Ordinal is both faster and more correct as our intent is to do … (dotnet#16439)
  merge (dotnet#16427)
  Optimize empty string compares (dotnet#16435)
  Checker: recover on unresolved type in 'inherit' member (dotnet#16429)
  Release notes proposal (dotnet#16377)
  [main] Update dependencies from dotnet/source-build-reference-packages (dotnet#16411)
  Allow usage of [<TailCall>] with older FSharp.Core package versions (dotnet#16373)
  Parser: recover on unfinished 'as' patterns (dotnet#16404)
  Parens: Keep parens in method calls in dot-lambdas (dotnet#16395)
  Checker: check unfinished obj expression inside computations (dotnet#16413)
  Added default dotnet-tools + additional tasks to launch them (dotnet#16409)
  make `remarks` and `returns` visible in quick info (dotnet#16417)
  ...
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.

5 participants