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

Fable support #688

Closed
wants to merge 4 commits into from
Closed

Fable support #688

wants to merge 4 commits into from

Conversation

ncave
Copy link
Contributor

@ncave ncave commented Jan 21, 2017

  • Mostly #ifdefs added, but not too many (IMO)
  • Uses a custom build step for code generation: fcs\build CodeGen.Fable
  • Parsing multiple files works
  • Untyped AST parsing works
  • Typed AST parsing also works
  • Compiling Fable with Fable works
  • Fable2 Demo REPL page works (Fable1 REPL here)
  • Next Step: To be determined by community, open for suggestions.

See also: Fable JS

@dsyme
Copy link
Contributor

dsyme commented Jan 22, 2017

@ncave Extraordinary. I'll take a close look

Is the demo page hosted somewhere? How big is the overall JS for the bootstrapped compiler?

Random future thought: Do you think there's any chance we could eventually produce .NET binaries with the JS compiler (and send them to .NET clients?) I've no idea when/where that would be useful, it's just a thought.

Also, I wonder if this might eventually make it into the Microsoft Visual F# repo and the standard F# command line compiler. That would be quite interesting :) Or is it always best to package it via Fable?

Export.FCS.sln Outdated
@@ -0,0 +1,27 @@
Microsoft Visual Studio Solution File, Format Version 12.00
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably best if all files that have something to do with Fable have Fable in the name where possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's not really related to the PR, it's just a quick hack to export metadata using FCS, so it should be moved out.

build.fsx Outdated
@@ -272,14 +272,27 @@ Target "CodeGen.NetCore" (fun _ ->
let fsLex fsl out = runInDir (toolDir + "fslex.exe") "%s --unicode %s -o %s" fsl lexArgs out
let fsYacc fsy out m o = runInDir (toolDir + "fsyacc.exe") "%s %s %s %s %s -o %s" fsy lexArgs yaccArgs m o out

#if FABLE_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to mention that in the visualfsahrp (and soon fsharp, and soon this) repo FsSrGen has been replaced by a script fssrgen.fsx

Copy link
Contributor Author

@ncave ncave Jan 23, 2017

Choose a reason for hiding this comment

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

I know, it's there just until the new tooling gets released and FCS is migrated to use it.

build.fsx Outdated
// comments the #line directive as it is not supported by Fable
["lex.fs"; "pplex.fs"; "illex.fs"; "ilpars.fs"; "pars.fs"; "pppars.fs"]
|> Seq.map (fun fileName -> IO.Path.Combine (workDir, fileName))
|> RegexReplaceInFilesWithEncoding @"# (?=\d)" "//# " Text.Encoding.UTF8
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason this doesn't work in fable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fable uses the line directive already to do source mapping, so we can't. The simplest way is to just comment it out.

build.fsx Outdated
// comments the #line directive as it is not supported by Fable
["lex.fs"; "pplex.fs"; "illex.fs"; "ilpars.fs"; "pars.fs"; "pppars.fs"]
|> Seq.map (fun fileName -> IO.Path.Combine (workDir, fileName))
|> RegexReplaceInFilesWithEncoding @"# (?=\d)" "//# " Text.Encoding.UTF8
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason this doesn't work in fable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see this.

src/absil/il.fs Outdated
@@ -8,14 +8,18 @@ module (*internal*) Microsoft.FSharp.Compiler.AbstractIL.IL


open Internal.Utilities
#if FABLE_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

You may as well make this unconditional e.g.

open Microsoft.FSharp.Collections // needed for Fable
open Microsoft.FSharp.Core // needed for Fable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be a good idea to retain the conditional as there is some type overwriting that is only needed when compiled with Fable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use FSLANG_NO_IMPLICITOPEN to indicate a dialect of F# that doesn't have implicit opening of these namespaces, thanks

Copy link
Contributor Author

@ncave ncave Feb 4, 2017

Choose a reason for hiding this comment

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

It's not that there is no implicit opening, it's the order of opening after the adapters are loaded which overwrites some core types with custom implementations.

src/absil/il.fs Outdated
open Microsoft.FSharp.Compiler.AbstractIL
open Microsoft.FSharp.Compiler.AbstractIL.Diagnostics
open Microsoft.FSharp.Compiler.AbstractIL.Internal
open Microsoft.FSharp.Compiler.AbstractIL.Internal.Library
open System.Collections
open System.Collections.Generic
open System.Collections.Concurrent

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove whitespace cahnges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

src/absil/il.fs Outdated
@@ -77,21 +81,21 @@ let rec splitNamespaceAux (nm:string) =

/// Global State. All namespace splits ever seen
// ++GLOBAL MUTABLE STATE
#if FABLE_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not conditionally define

type ConcurrentDictionary<'K,'V> = Dictionary<'K'V>

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, just looked cleaner this way. This too can go away after Dictionary constructors can be handled properly by Fable.

src/absil/il.fs Outdated
(x.Scope = y.Scope) &&
(x.Name = y.Name) &&
(x.Enclosing = y.Enclosing)
override x.Equals(yobj:obj) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed? When would the cast fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall the actual reason, probably unsupported casting by Fable.

src/absil/il.fs Outdated
member x.IsNewSlot = match x.mdKind with | MethodKind.Virtual v -> v.IsNewSlot | _ -> invalidOp "not virtual"
member x.IsCheckAccessOnOverride= match x.mdKind with | MethodKind.Virtual v -> v.IsCheckAccessOnOverride | _ -> invalidOp "not virtual"
member x.IsAbstract = match x.mdKind with | MethodKind.Virtual v -> v.IsAbstract | _ -> invalidOp "not virtual"
member x.IsFinal = match x.mdKind with | MethodKind.Virtual v -> v.IsFinal | _ -> false //invalidOp "not virtual"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changes needed? I see they are pretty harmless changes though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revisit that.

src/absil/il.fs Outdated
@@ -3661,7 +3666,11 @@ type ILTypeSigParser(tstring : string) =
// fetch the arity
let arity =
while (int(here()) >= (int('0'))) && (int(here()) <= ((int('9')))) && (int(peek()) >= (int('0'))) && (int(peek()) <= ((int('9')))) do step()
#if FABLE_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just always use the Fable version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I just tried to be conservative with changes to lower the impact.

src/absil/il.fs Outdated
@@ -1552,7 +1557,7 @@ and [<Sealed>] ILTypeDefs(f : unit -> (string list * string * ILAttributes * Laz
let mutable array = InlineDelayInit<_>(f)
let mutable dict = InlineDelayInit<_>(fun () ->
let arr = array.Value
let t = Dictionary<_,_>(HashIdentity.Structural)
let t = Dictionary<_,_>(3, HashIdentity.Structural)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's up here - doesn't Fable have a default size constructor?

Copy link
Contributor Author

@ncave ncave Jan 23, 2017

Choose a reason for hiding this comment

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

Main reason is Fable (and Javascript maps/sets) have no support for collections with equality comparers. Technically map/set trees with IComparer can be used as a substitute, but probably the performance will not be too good, more like a list instead of a map (not sure how important collection performance is here though). Very few places in FCS actually use a custom equality comparer, so for now we're getting away with just ignoring them, that's one of the corners that have been cut and has to be fixed. Technically this should eventually go away as it's just a deficiency in Fable.

@dsyme
Copy link
Contributor

dsyme commented Jan 22, 2017

Made a few initial comments. I suppose the big challenge is going to be keeping this compiling and working as changes flow in from the Visual F# TOols

src/absil/il.fs Outdated
@@ -1552,7 +1557,7 @@ and [<Sealed>] ILTypeDefs(f : unit -> (string list * string * ILAttributes * Laz
let mutable array = InlineDelayInit<_>(f)
let mutable dict = InlineDelayInit<_>(fun () ->
let arr = array.Value
let t = Dictionary<_,_>(HashIdentity.Structural)
let t = Dictionary<_,_>(3, HashIdentity.Structural)
Copy link
Contributor

Choose a reason for hiding this comment

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

should 3 be arr.Length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but that's not the default value for this contructor. In any case, this will probably go away, as it's just a work around a deficiency in Fable (can't do comparers), when it's resolved I'll take it out.

src/absil/il.fs Outdated

member aref.QualifiedName =
let b = new System.Text.StringBuilder(100)
let add (s:string) = (b.Append(s) |> ignore)
let addC (s:char) = (b.Append(s) |> ignore)
let addC (s:char) = (b.Append(string s) |> ignore)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure it impacts much the perf but maybe make a conditional for this as well?

Copy link
Contributor Author

@ncave ncave Jan 23, 2017

Choose a reason for hiding this comment

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

Don argued against making it conditional here.

@ncave
Copy link
Contributor Author

ncave commented Jan 23, 2017

@dsyme @smoothdeveloper Thank you for your comments!
Some of the unexplicable ifdefs are just to get around deficiencies in the compiler, while some others are actually a problem in FCS (see #676 for example, which is forcing the addition of dummy constructors all over the place).

@dsyme

  1. Demo page is not hosted anywhere yet but you can host it locally with Node after it's compiled with Fable. The bundled JS size is around 8 MB (unminified) so it should get smaller when minified. Compilation time for scripts is not as bad as I thought it would be, just a few seconds, but since the whole thing is completely unoptimized, it should get better when the hot path is optimized.

  2. Sure, I see no technical reason to not be able to produce .NET binaries with the JS compiler. Not sure if that's applicable in a browser setting, but perhaps in Node setting. A bit more of FCS will need to be compiled, but shouldn't be a problem if there is interest.

  3. For a full-featured replacement as a tool it will need the rest of FCS to be compiled as well (file system, build system, project support, etc.), but yes, no reason why it can't be migrated to the upstream repos.

@dsyme dsyme closed this Jan 30, 2017
@dsyme dsyme reopened this Jan 30, 2017
@dsyme
Copy link
Contributor

dsyme commented Jan 31, 2017

@ncave Looks like this is green :) Do you have more work you'd like to do on this, or should we work towards a final review?

@ncave
Copy link
Contributor Author

ncave commented Jan 31, 2017

@dsyme The PR can probably use some work to clean it up a bit, but if I recall correctly, you mentioned that there are some large upstream changes incoming, so it may be a good idea to wait until those are merged in and adapt the PR to them before moving forward.

@ncave ncave force-pushed the fable branch 4 times, most recently from 63c12b9 to e646aa8 Compare February 1, 2017 07:14
@@ -0,0 +1,21 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be included in the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take it out. It was just handy to have a debugging environment out of the box, but it does not belong in the PR.

let (===) x y = LanguagePrimitives.PhysicalEquality x y

#if !FABLE_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be better to use a conditional based on feature like #if FX_NO_GETCURRENTPROCESS or the like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like FX_NO_GETCURRENTPROCESS is not used anywhere else. I've tried to keep the conditionals to a minimum and not introduce new ones unnecessarily. IMO, there are quite a few BCL APIs not supported by Fable, it would become crowded very fast if we introduce a different conditional for each one that is not supported. But if you feel strongly about it, I'll change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally I'd like the F# compiler codebase to not mention Fable at all , but rather be ``#if`'d on a set of characteristics that Fable happens to have. I'm expecting that there will other F#-to-XYZ attempts in the future, e.g. to the JVM, or another F#-to-Javascript compiler (e.g. WebSharper), or F#-to-native.

The more we can use FX_NO_XYZ... or FSLANG_NO_BYREFS the more the compiler code will be "neutral" with respect to these different efforts.

Copy link
Contributor

@dsyme dsyme Feb 4, 2017

Choose a reason for hiding this comment

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

To give context: we've had this before, e.g. for Silverlight we scattered hundreds of #if SILVERLIGHT across the codebase. They hung around for years and years even after we stopped cross-compiling - and we had cause to keep some of them because some carried across to .NET Core, some were feature-specific etc. The lesson I learned from that is to always ask people to be specific about why #if creep in: if there are just 5-10 uses of an `#ifit's ok, but there are about 180 uses ofif FABLE_COMPILER`` - so we should take the time to divide them into the true underlying reasons now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine, if that's your preference I'll try to replace all #if with feature conditionals.

/// An efficient lazy for inline storage in a class type. Results in fewer thunks.
#if FABLE_COMPILER
type InlineDelayInit<'T when 'T : not struct>(f: unit -> 'T) =
Copy link
Contributor

Choose a reason for hiding this comment

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

When you use #if FABLE_COMPILER please add a comment indicating what Fable doesn't support, or use a feature define like #if FSLANG_NO_MUTABLE_STRUCTS. The latter helps document that the F# language as supported by Fable has some specific semantic differences to F# as supported by .NET.


let order = LanguagePrimitives.FastGenericComparer<string>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove whitespace changes where possible

@@ -808,10 +831,17 @@ type LayeredMap<'Key,'Value when 'Key : comparison> = Map<'Key,'Value>
type Map<'Key,'Value when 'Key : comparison> with
static member Empty : Map<'Key,'Value> = Map.empty

#if FABLE_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

Add explanatory comment for the #if please

@dsyme
Copy link
Contributor

dsyme commented Sep 26, 2018

Thanks all, see https://github.com/fable-compiler/repl2/issues/48 :)

@alfonsogarciacaro
Copy link
Contributor

@ncave It seems the fsharp/FSharp.Compiler.Service repo is lagging behind Microsoft/visualfsharp. Would it be possible to change your fork to that repo? Does it make any sense at all?

@ncave
Copy link
Contributor Author

ncave commented Jan 28, 2019

@alfonsogarciacaro This has been discussed before. Of course we can, and would be nice to not have to wait until latest (and greatest) gets merged downstream, but there is very little chance (if any) for this PR to ever be merged over there. Not that it has much better chances here either (for understandable reasons), so yeah, perhaps we can just open a new one in a Microsoft/visualfsharp fork without submitting a PR. I'll consider it.

@ncave ncave force-pushed the fable branch 4 times, most recently from ddbeea5 to bef8db3 Compare February 2, 2019 22:20
@ncave
Copy link
Contributor Author

ncave commented Feb 2, 2019

Rebased to latest (26.0.1).

@ncave
Copy link
Contributor Author

ncave commented Feb 23, 2019

Rebased to latest.

@ncave
Copy link
Contributor Author

ncave commented Feb 26, 2019

Rebased to latest (27.0.1).

@ncave ncave force-pushed the fable branch 2 times, most recently from c3cd85d to fd3a482 Compare March 23, 2019 17:43
@ncave
Copy link
Contributor Author

ncave commented Mar 29, 2019

Rebased to latest (28.0.0).

@ncave
Copy link
Contributor Author

ncave commented May 20, 2019

Closing this PR in favor of this one upstream.
It was getting long in the tooth anyway ... 😀

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.

9 participants