Skip to content

Conversation

@cartermp
Copy link
Contributor

@cartermp cartermp commented Oct 12, 2018

Do not merge until we're ready to publish a new FSharp.Core and/or language version.

Implements fsharp/fslang-design#337

  • Added tests
  • Removed some redundant functions used internally
  • Cleaned up some bizarre whitespace formatting in option.fs

Also, lesson learned: NUnit doesn't do a very good job of working with value types.

@cartermp
Copy link
Contributor Author

So this breaks the FCS build, since it was relying on the internal functions. Is there a way around that? Seems strange that we can't update FSharp.Core without breaking FCS if you replace internal equivalents with the proper FSharp.Core version of something.

@dsyme
Copy link
Contributor

dsyme commented Oct 15, 2018

So this breaks the FCS build, since it was relying on the internal functions. Is there a way around that? Seems strange that we can't update FSharp.Core without breaking FCS if you replace internal equivalents with the proper FSharp.Core version of something.

FCS gets built against a known, published FSHarp.Core. This is for the usual reasons: we want to be able to publish FCS at any point and have it usable with a known, reliable FSHarp.Core dependency (and one that doesn't necessarily force a package upgrade on downstream users - it's a package not an application).

I don't think this approach should change so it's just something we have to live with when authoring compiler code.

You can make things conditional using COMPILER_SERVICE_AS_DLL or add some new define like FX_PUBLISHED_FSCORE

@cartermp
Copy link
Contributor Author

Yeah, I'd much rather not introduce a flag for this. We'll just merge this when we're ready to publish a new version of FSharp.Core.

@cartermp cartermp changed the title [WIP] FS-1065 implementation FS-1065 Value Option Parity Oct 15, 2018
@cartermp cartermp added this to the 16.0 milestone Oct 15, 2018
@cartermp
Copy link
Contributor Author

cartermp commented Oct 15, 2018

Okay, I've tagged this for vNext and assigned the appropriate milestone. @KevinRansom / @TIHan / @dsyme we'll leave this one open (even if approved) until we're ready to start merging in F# vNext PRs.

@dsyme
Copy link
Contributor

dsyme commented Oct 18, 2018

Yeah, I'd much rather not introduce a flag for this. We'll just merge this when we're ready to publish a new version of FSharp.Core.

Please add the flag, it's the right way to do it and a necessary part of the PR. We have good engineering reasons to treat FCS as a library package and thus minimize its FSHarp.Core dependency, rather than maximize it, according to our standard guidance for library packages. We will likely want FCS to have a 4.5.2 dependency for a very long time.

The fact we happen to build an application (fsc.exe etc.) using FCS should not mean we treat FCS as an application.

@cartermp
Copy link
Contributor Author

I expect that we would rev the package dependency of FCS any time we have language changes that also require core library additions though, right? I'll add a directive, though this sort of stuff gets awfully difficult to work with over time...

@dsyme
Copy link
Contributor

dsyme commented Oct 19, 2018

I expect that we would rev the package dependency of FCS any time we have language changes that also require core library additions though, right? I'll add a directive, though this sort of stuff gets awfully difficult to work with over time...

FCS continues to have a min dependency of the lowest FSHarp.Core possible to allow it to be hosted in the broadest range of applications. Historically that has been assembly version 4.4.1.0 but we bumped to 4.3.4 for netstandard2.0. We should honestly stay on 4.3.4 for as long as possible but forki bumped to 4.5.2, though that was an anomoly and we shouldn't get into the habit of doing that.

If there is a compelling technical reason (beyond dogfooding) to adopt the latest language+library features in implementing the compiler then we bump, otherwise we stay low.

@dsyme
Copy link
Contributor

dsyme commented Oct 19, 2018

though this sort of stuff gets awfully difficult to work with over time...

It's not too hard :) The FCS part of the compiler must be implemented with a specific FSharp.Core. Lots of people have to implement things with a specific FSHarp.Core :)

The mistake in the PR is assuming that types in the compiler can be removed just because FSHarp.Core has been improved. You don't have to remove those types from the compiler as part of the PR.

@cartermp
Copy link
Contributor Author

@dsyme I ended up calling it ValueOptionInternal due to my allergy to adding another directive. Does that work?

@dsyme
Copy link
Contributor

dsyme commented Oct 30, 2018

Perfect

@KevinRansom KevinRansom merged commit 44c7e10 into dotnet:master Nov 13, 2018
KevinRansom pushed a commit that referenced this pull request Nov 13, 2018
* Remove dependence on runfsc.cmd and runfsc.sh (#5882)

* Remove dependence on runfsc.cmd and runfsc.sh

* White space

* Add TryExactlyOne for array, list and seq. (#5804)

* FS-1065 Value Option Parity (#5772)

* Initial FS-1065 implementation

* Undo removal of compilationrepresentation suffix and update surface area

* Whoopise, add the suffix to the impl file

* Update coreclr surface area

* Revert the FSComp changes that somehow got picked up

* newline

* Consume internal VOption module functions

* More internal voption module functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants