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

update FsLexYacc to 8.0.1 source (by direct copy) #6355

Merged
merged 5 commits into from
Mar 29, 2019

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Mar 21, 2019

I believe this should fix bug #6346, as discussed in the comments there. It gets rid of the reliance on .NET taking all mutually-recursive tailcalls for basic lexing. It should also improve the performance of the lexer a bit.

Note this is a direct-copy update of the files in "buildtools" to match FsLexYacc 8.0.1

@dsyme
Copy link
Contributor Author

dsyme commented Mar 21, 2019

@KevinRansom We should consider how we keep the buildtools files in sync with FsLexYacc - we added the source to the nuget package to allow this, but after recent build updates to remove packages.config in visualfsharp we don't acquire or record the package version we have copied over at all....

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

A couple namespaces missed

src/buildtools/fslex/fslex.fs Show resolved Hide resolved
src/buildtools/fsyacc/fsyacc.fs Show resolved Hide resolved
@dsyme
Copy link
Contributor Author

dsyme commented Mar 21, 2019

This is ready.

@baronfel
Copy link
Member

baronfel commented Mar 22, 2019

As soon as this merges I'll start flowing a new build/release of FCS/FSAC so that we can test the fix in editor tooling on nonwindows

actions |> Seq.iteri (fun i (code,pos) ->
cfprintfn os " | %d -> ( " i;
cfprintfn os "# %d \"%s\"" pos.Line pos.FileName;
cfprintfn os "// Rule %s" ident
Copy link
Member

Choose a reason for hiding this comment

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

This must be the change where it fixes the issue.

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 that's right

@dsyme
Copy link
Contributor Author

dsyme commented Mar 28, 2019

@TIHan Can we have an approve on this please? Thanks

@alfonsogarciacaro
Copy link
Contributor

I tried cherry-picking e5fcd48 into the FCS fork we're using, did a clean build and copied the FSharp.Compiler.Service.dll to Fable. But I'm still getting SO exceptions when compiling big projects (FCS+Fable itself) on macOS :/

@dsyme
Copy link
Contributor Author

dsyme commented Mar 28, 2019

I tried cherry-picking e5fcd48 into the FCS fork we're using, did a clean build and copied the FSharp.Compiler.Service.dll to Fable. But I'm still getting SO exceptions when compiling big projects (FCS+Fable itself) on macOS :/

Right, but is it the same SO stack?

If fewer tailcalls are being taken then that will always cause problems on bigger projects. This rooted out one specific problem, but I expect you'll see other ones. We can possibly make progress one by one, but it would be much better to have the CLR honour the tailcall attributes consistently.

@alfonsogarciacaro
Copy link
Contributor

Not sure, don't know how to get the dump on macOS (I don't see the error on Windows). It happens when calling ParseAndCheckProject so I assume this is coming from FCS. On the Fable side, @ncave added a trampoline some time ago to avoid excessive recursion and it's been working so far.

I've been switching the last months between mac and Windows for Fable development so I cannot recall when this started to happen. In CI, Fable only builds itself in Appveyor (Windows) so we haven't seen the issue.

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.

None yet

7 participants