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

Make Fable AST independent of FCS #2138

Merged
merged 8 commits into from
Aug 22, 2020
Merged

Conversation

alfonsogarciacaro
Copy link
Member

This was supposed to fix #2087 but then I started another AST cleanup, tried to make Fable.AST independent of FCS and then everything become red 😮 Finally the project builds again but I need to fix the tests. It's gonna be difficult to make a proper review because the PR is too big and the reasons behind many of the changes are only in my head atm, but if you have a moment @ncave please do have a look to see if this makes sense to you.

Next steps I think this could unblock:

  • Run formatter on the files Fable.Transform. Most of the lines have been touched anyways.
  • Compile to class types
  • Don't use inheritance for equality/comparison as discussed here: [WIP] Optimized List module #2124 (comment)
  • Move Fable.AST to a separate project, which should be available to plugins
  • Move Replacements out of Fable.Transforms, and make them look more like "regular" plugins. Hopefully this should help make the compiler more modular and easier to maintain/contribute

@gitpod-io
Copy link

gitpod-io bot commented Aug 18, 2020

@ncave
Copy link
Collaborator

ncave commented Aug 18, 2020

@alfonsogarciacaro Looks good, it will eliminate a lot of impedance and unnecessary pasta code that was previously needed because Fable AST was missing some elements of FSharp AST, hence the need to keep and inspect both at later stages. Since this is an internal change, in theory we should be able to resolve all errors and restore full functionality before other big changes are introduced.

@ncave
Copy link
Collaborator

ncave commented Aug 22, 2020

@alfonsogarciacaro
Here is an issue with this branch, only happens if there are 2 separate module files:

module Library
let res = 0
open Library
let test = fun () -> res

[<EntryPoint>]
let main argv =
    test ()

compiles to:

import { res } from "./Library.js";
export const test = res;

(function (argv) {
  return test(); // <-- Error: test is not a function
})(process.argv.slice(2));

Could be some sort of optimization issue, not sure.

@alfonsogarciacaro
Copy link
Member Author

Oh, my, had I seen your comment before I could have saved a lot of time trying to pinpoint the issue. Yes, it was this: ea45ca1#diff-642c2bef493bb0bb4c567fd3b854b15dL956

That's the code handling "import expressions" as in: let foo (x: int): string = importMember "./util.js". Because of my changes in the AST, it was thinking that the internal import (generated by the compiler) was a user-generated import. To fix it I added a IsCompilerGenerated property to imports. Although tbh, I'm not sure why it wasn't failing before.

Anyways, this is finally green! It's not perfect yet, but let's merge it so we can move forward with Nagareyama 👍

@alfonsogarciacaro alfonsogarciacaro merged commit 569bfa6 into nagareyama Aug 22, 2020
@alfonsogarciacaro alfonsogarciacaro linked an issue Aug 24, 2020 that may be closed by this pull request
@alfonsogarciacaro alfonsogarciacaro deleted the standalone-ast branch October 1, 2020 02:15
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.

Better names in JS
2 participants