Skip to content

Add analyzer to prevent use of blocking synchronous APIs#96

Closed
Smaug123 wants to merge 6 commits intoG-Research:mainfrom
Smaug123:non-blocking
Closed

Add analyzer to prevent use of blocking synchronous APIs#96
Smaug123 wants to merge 6 commits intoG-Research:mainfrom
Smaug123:non-blocking

Conversation

@Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Aug 27, 2025

The "Initial implementation" and "Tests" commits were almost entirely produced by Claude Opus.

This PR is something of a request for comment; it's not completely clear to me that it's a desirable analyzer in general. But we've been caught out a couple of times recently by this.

@@ -1,41 +1,42 @@
<?xml version="1.0" encoding="utf-8"?>
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scaffolding script reformatted this file.

<Compile Include="ImmutableCollectionEqualityAnalyzerTests.fs" />
<Compile Include="TypedInterpolatedStringsAnalyzerTests.fs" />
<Compile Include="DisposedBeforeAsyncRunAnalyzerTests.fs" />
<Compile Include="SyncBlockingAnalyzerTests.fs" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only actually-changed line, and it's new.

@Smaug123
Copy link
Contributor Author

Interested in what @G-Research/rqf have to say on this

@Smaug123 Smaug123 marked this pull request as ready for review August 27, 2025 16:30
@nojaf
Copy link
Member

nojaf commented Aug 28, 2025

almost entirely produced by Claude Opus.

Damn, that is quite impressive. This looks good.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Aug 28, 2025

Damn, that is quite impressive. This looks good.

It helps that the analyser is very simple and is similar to existing ones. It duplicated the comment-suppression code which I manually factored out into that other PR, and it took quite a bit of (autonomous) trial and error to write the tests; I told it to mimic the existing tests, but its first attempt was broken in a number of ways (eg it got the wrong names for Async.RunSynchronously) and it took perhaps ten minutes to fix it by iterating on an fsx file.

In general I have had pretty poor results from Opus when it comes to actually writing code in F#, although it has a better knowledge of the dotnet runtime than all but the best maintainers, I'd say - its internal knowledge completely eclipses mine, so I generally use it as a vastly better interactive documentation source.

let Code = "GRA-SYNCBLOCK-001"

[<Literal>]
let SwitchOffComment = "synchronous blocking call allowed"

Choose a reason for hiding this comment

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

Is the scope of this comment the next line or something broader (or narrower)?

For example, would I have to do?:

foo
    bar
// synchronous blocking call allowed
    baz.Result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's currently specific to the line immediately before. We could totally do better.

@kojo12228
Copy link

This is cool, I can't think of another case where it would useful to call .Result (maybe in tests for mocked functions that are async but end up doing nothing, but as you point out the text can simply be made async as well)

@Smaug123
Copy link
Contributor Author

I've implemented this in WoofWare.FSharpAnalyzers, which is more opinionated; I'll close this.

@Smaug123 Smaug123 closed this Nov 21, 2025
@Smaug123 Smaug123 deleted the non-blocking branch November 21, 2025 07:04
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.

3 participants