Supports async-parsing #771
Conversation
|
LGTM, can you fix CI? Cheers! |
Sorry! CI was broken because of a different problem, which was addressed in #769 (which I just merged). Can you rebase? |
|
@Thorium now it's giving an error on SelfCheck target, rule AvoidSinglePipeOperator which is not enabled by default, but for SelfCheck we now try to enable all rules, can you have a look? It's just that this rule considers that if you don't use the pipe operator in a chained way, then there's no point to use it (so, more readable and less verbose to have |
| val lintSource : optionalParams:OptionalLintParameters -> source:string -> LintResult | ||
|
|
||
| /// Lints F# source code async. | ||
| val lintSourceAsync : optionalParams:OptionalLintParameters -> source:string -> Async<LintResult> |
There was a problem hiding this comment.
@Thorium actually, the convention in .NET is that if a method has an "Async" suffix then it returns Task<Foo>, not Async<Foo>. So let's rather rename it to asyncLintSource please.
There was a problem hiding this comment.
That's true, but another question is that should all the F# lint use task { } instead of async { } because lintting is a small task and not a big asynchronous workflow (with different continuation branches or web-request error handling etc). However, task has bit better performance but I have to say performance has never been issue for me in fsharplint, it always runs within few seconds, so maybe it wouldn't be worth of changing the API.
There was a problem hiding this comment.
I like the fact that one of the latest releases of F# included this new CE task, but I think the performance advantages of it are not worth it because of the maintainability issues it has over async (e.g. see https://tomasp.net/blog/csharp-async-gotchas.aspx/ ). I mean, if you have to use Tasks because you're interoperating with C#, then using this native CE might be good, but for idiomatic F# that doesn't need to call C#, there's no point. In fact, I've recently stated this properly in our Copilot instructions in this recent PR: 74962b8
BTW, I guess you noticed that I pushed into your fork's branch that this PR targets, because I was planning to apply the suggestion I made myself and merge it right away; however, after pushing, two things happened:
- Somehow CI was red (maybe because of a bad rebase, I'm not sure), so @webwarrior-ws had to push an additional commit to make compilation green (please double check if it's good on your side).
- Scheduled CI (that we run once a day) started failing for the master branch, so I want to push a fix for that (e.g. which might just be Numpsy's PR Fix build failures when building with the .NET 10 SDK #779 as soon as he removes the "Draft" marking) before merging this.
There was a problem hiding this comment.
Yes, feel free to modify the PR.
There are a few dis-advantages on async:
- Performance. Both execution speed ad memory usage.
- Async builds on stack, so the functions you think being tail-recursive are not. Stackoverflow possible
- Error handling:
asyncswallows the exceptions by default, wheretaskthrows them. Withasyncthere is no stacktrace to tell what happened (because there is no thread), but withtaskit still maintains some level stacktrace. - If you forget to explicitly start the
async, then nothing happens and the computation is forgotten. So juniors think their code works when in real life it's never even called.
...but yes it's still usable concept and an alternative. When things get really complex (should they ever?), task tends to fail.
There was a problem hiding this comment.
Stackoverflow possible
Argh, didn't know this. Maybe the solution here is launching them as ValueTasks?
Error handling: async swallows the exceptions by default
Mmm, are you sure? I think it depends. If you use Async.RunSynchronously, they don't, IIRC. But indeed one has to be careful with things like Async.StartAsTask.
If you forget to explicitly start the async, then nothing happens and the computation is forgotten
I'd say the best protection from this is enabling warnings as errors; thanks to this, juniors would have to ignore the result of the async job explicitly in order to shoot themselves in the foot (unless, the Async job returns unit, in which case we're screwed, indeed).
There was a problem hiding this comment.
If you use Async.RunSynchronously, they don't,
Correct.
juniors would have to
ignorethe result of the asyn
This is what our junior did, but it was wrong, it should have been Async.ignore for the computation to run.
There was a problem hiding this comment.
This is what our junior did, but it was wrong, it should have been Async.ignore for the computation to run.
Ah! Maybe in that case our rule70 (https://fsprojects.github.io/FSharpLint/how-tos/rules/FL0070.html) would have helped ;)
| let generateAst source = | ||
| let checker = FSharpChecker.Create(keepAssemblyContents=true) | ||
| let sourceText = SourceText.ofString source | ||
| let generateAstAsync source = |
knocte
force-pushed
the
async-parsing
branch
2 times, most recently
from
5066805 to
5fdb6e0
Compare
Suffix "Async" for methods is for methods that return "Task<Foo>", not "Async<Foo". For more info, see the PR for an upcoming FSharpLint rule: fsprojects#586
knocte
force-pushed
the
async-parsing
branch
from
aefc892 to
d0627fb
Compare
3 participants
Expose Async API for parsing.
This is useful when FSharpLint is used by other means than console app.