-
Notifications
You must be signed in to change notification settings - Fork 301
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
Added ParseAndCheckFileInProject #1714
Conversation
@ncave Awesome 🎉 And ok message received I will see what I can do about that :) |
Fantastic! 👏 👏 Thank! |
Just to clarify, there are 3 parse and check methods now:
|
Thanks @ncave! Can we add this information as doc comments in the actual methods and if possible also add some information in the method name itself (e.g. |
Another question: the If this doesn't work I guess we need to create our own cache in the |
@alfonsogarciacaro Raising the REPL bar from multi-file to multi-project, I see :) I like it! Currently the Technically you can reuse the About reusing references between multiple projects, do you mind describing the use case for it? |
Actually I'm playing with the idea we discussed before about using your fork directly instead of the FCS package even in the Fable CLI (dotnet). Because the CLI acts as a server and because Webpack can have multiple project entries (or you can even import another .fsproj from F# using Webpack as the REPL itself does) we've to support compilation of multiple projects. Another issue I'm also having is the Anyways, if you think this doesn't make sense, we can go back and make Fable.Cli depend on the FCS package. I was just trying to uniform the different components and if you finally redirect your fork to visualfsharp it could be a way to get enhancements faster. But these are not showstoppers :) |
@alfonsogarciacaro You can still use my fork, or any other fork, or no fork (directly), if you are project-referencing FCS (by source from Microsoft/Visualfsharp, so you get latest). IMO Fable on dotnet should be project-referencing stock FCS, which is the officially supported tooling, used by everybody else, and guaranteed to work. Since Fable on dotnet is using dotnet anyway, and dotnet build resolutions, there is no reason not to. I don't see any benefit in trying to unify the last step (the CLI), as it inevitably will degrade quality and reliability.
|
The "without the .dll" part is a historic artifact, back from the olden days a few years back when FSharp.Core.dll was still distributed with a signature file on the outside. This can be changed. |
I was hoping we could take advantage of the improvements you added, like skipping the symbol uses to reduce the memory pressure, using hashes to cache the file contents, etc. With the "standard" FCS there are several things I don't understand well and I'm not sure if I'm doing things correctly. For example, for the first compilation I use In any case, for now I just added a workaround for the issue with the dll paths and it's working! 🎉 I will test it tomorrow with some of my projects to see if watch compilation times improve. |
Some of those improvement may be already there in recent changes in MS F# (like using source hashes in cache keys), so they will eventually trickle down to FCS. |
Hmm, I'm getting issues with the fileNames on Windows :/ So I guess we'll have to stick with "official" FCS as you said :) I'll try to compile it directly from the visualfsharp repo to see if we can support anonymous records. |
@alfonsogarciacaro What kind of issues? The paths in file names need to be normalized? |
For example, when building the library on Windows, at this point I'm passing the following file names:
But the implementation files returned are only:
Those in a subfolder or with a dot in the name are missing. It was working on macOS yesterday so I assume it has to do with the normalization of the path separators, |
That may not be it the whole story, why is then Global.fs not there. |
@ncave Some more findings/questions:
|
@alfonsogarciacaro We can pass full project options instead, but then the question is do we pass it on InteractiveChecker construction, or on the parsing call, or both? And when it changes, do we want to invalidate all cache, or just some, or none? (there is a scenario that you add a file to a project and you don't want to recompile the whole thing just because of that, not sure how common that is). I guess we can separate the project file list from the rest of the options, that seems more flexible. |
Right now (in Also, now Fable creates the checker just once and reuses it every time. The options are passed when parsing the project (or a single file). But if it helps with the caches, it's ok for me to associate one checker with each project. Actually, this is how I'm doing it in the As a note, sometimes a couple of independent projects can share one file as it happens with the REPL Worker. Not sure if that can affect caching.
If the issue with the drive latter in the file names can be fixed too that'd be fantastic :) |
I think I fxed it, get latest. You shoudn't need to remove the paths from references anymore, or remove .dll extensions. I tried to make it backwards compatible, if reference has no .dll extension, will add it, if there is, leave it as is. |
Thanks @ncave! I will give it a try tomorrow... a new FCS package has been finally published so I need to give anonymous records a try ;) About the issue with the file names on Windows. I'm totally puzzled. I browsed a bit your code and got suspicious of implicitIncludeDir but I tried to set it to different values and it didn't help. It's very weird, if I pass the file names without removing |
No luck with anonymous records. Seems the expression tree wasn't updated so we still have to wait :/ I'm trying to get a better grip of the FCS code base. I run { FilePath: string; FileContent: string; Version: int } Then you can use |
@alfonsogarciacaro The cost of hashing the source is probably trivial, compared to everything else. IMO we still need to make sure the source didn't change somehow, and the hashing is not a faster way to do that but a less memory intensive way, as it does not keep sources as keys (similar to this).
Thank you, I was going for simplicity and minimalism there (poor man's IncrementalBuilder :). |
Unfortunately needs to keep the sources in memory to avoid to reread all of them from disk every time a new watch compilation is triggered, which kind of defeats the purpose. Maybe the sources could be lazy and we could pass an The other alternative would be to pass |
@alfonsogarciacaro I'm not sure I understand what is the optimization you're going for, what is that change going to save or simplify vs the existing functionality. Perhaps an example will help. |
@ncave Nothing in particular. I'm just excited after testing Fable with your code 😄 The best part of web development is hot reloading and with Fable we've come pretty close to that. But using FCS for that it's been always a bit painful and I'm not entirely happy with the current solution: using I was just thinking aloud if there was still room for improvements, but it's not that important. I'm just trying to understand better what's going on in |
@alfonsogarciacaro That's simple, all cached typecheck entries below the modified file are removed. |
@alfonsogarciacaro I changed the typecheck cache to copy the good entries, remove everything, then add them back. Also, the project errors are now grouped by file and sorted ascending by line and column. |
@ncave Awesome, thank you! Interesting, I really appreciate your explanations. So contrary to my intuition, parsing takes much longer than type checking and the secret of your technique is separating the parsed and type checked caches so you can make the former more efficient 💪 |
@alfonsogarciacaro I'm not sure why it's happening, but it's easy to fix, just remove the colon from the file name before sending it to the checker, i.e. |
@alfonsogarciacaro Alright, I created a new FCS branch just for you to experiment with :). It adds the |
@ncave It works and it's awesome :) I've published fable-compiler 2.2.0-beta using your second fork. Some of the watch compilations seem to be somewhat faster now but more importantly now users are always notified if a change causes an error in another file (sometimes these went missing if there was no a I only had issues with the warnings, as the slim service is not filtering them by level. I added a custom filter that seems to work though it's currently ignoring the options from the .fsproj. |
@alfonsogarciacaro did you try the sample with |
@nojaf Sorry, I only tested locally and didn't notice I had to change the assembly name when loaded from the package. Can you please try with 2.2.0-beta-004? |
It should, the default level is 3, every other warn-on or warn-off needs to come via options from the .fsproj (into FSharpProjectOptions). |
@alfonsogarciacaro it works a like a charm. One little regression I think is
It creates a warn log instead of an error. 2.1 does show the error. |
@alfonsogarciacaro @nojaf Seems like compiler warning options are properly applied to the parsing phase, but not to the typecheck phase logger. I'll look into it. |
In the FCS service there's a function named |
@ncave Can the service_slim deal with signature .fsi files? I'm having issues to compile the REPL (now fable-standalone). It's something similar to what happened before, the implementation files returning by the checker are missing some entries (when there's a .fsi/.fs couple of files, it seems to discard the .fs). For example, SymbolHelpers.fs file cannot be located in the map of implementation files. But this time is happening both on Windows and Linux (Travis). |
@ncave Seems to be an issue with the deduplication, if I comment out this line, then I can build the REPL. I assume this is not a solution as it will likely cause problems in other situations, but I don't know what's the correct way to call this. The IncrementalBuilder does something similar to service_slim but in service.fs the return value of moduleNamesDict is discarded. I doubt that's the answer for us, because moduleNamesDict starts just as an empty Map. |
@alfonsogarciacaro I'll take a look. Same code works perfectly fine in the other fork (the one with a lot of FABLE_COMPILER magic sprinkled on it :). |
|
@alfonsogarciacaro
|
@ncave Awesome, thanks a lot! Fable standalone is building now in Appveyor for the
Interesting. Seems a bit counter intuitive as the implementation files should be the .fs ones (and I wonder why it was working before). In any case I fixed it following your suggestion and it's working now, thanks again! |
works again with 2.2.0-beta-006, I see the red errors, but I think it works a bit too well. With |
Hmm, strange. Fable is supposed to hide warnings from files in |
@alfonsogarciacaro Is it because those warnings are converted to errors, and Fable is compiling the "pre-compiled" packages with the same compiler options? ( |
@ncave Packages in |
@alfonsogarciacaro Thanks for the information. @nojaf Looks like you have to use compiler options that work for all the components you use, since they all get compiled as one big project. |
BTW, one of the reasons to merge everything in a single project is because is not possible (or at least I didn't manage to do it) to inline functions across different projects. Fable first versions had this issue. |
So did Fable 2.1 also merge to one project? If not that would explain the change in behaviour if so I think we are missing something here. |
Sources are being merged in one single project since Fable 1 if I remember correctly :) Anyways, I think there's something different going on here, because the ReactServer file is surrounded with How are you compiling the project? If using fable-splitter you can pass |
I'm using fable-loader. The log below shows starting without errors in my code. Fable log
|
Ah, so this only happens in watch compilations! You should have started from there ;) It may be that some project options are not honored in watch compilations, let me check... |
@nojaf I cannot reproduce... |
Minimal repro: https://github.com/nojaf/fable-2.2-beta-repro |
Thanks @nojaf! I can reproduce and understand the issue now 👍 Can you please check if the latest beta fixes it? |
Beta 007 fixes the issue, many thanks! |
@alfonsogarciacaro @MangelMaxime This enables implementing a multi-file tree view pane in the REPL.