Skip to content

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Oct 14, 2025

See tracking issue #18989

Merged #18985 #18986 #18990 to see if it all together still works.

TODO:

  • restore switches
  • double check that with everything off things still work

Some il baselines changed because of reordered methods.
This also mostly does not work with deterministic compilation.

Copy link
Contributor

github-actions bot commented Oct 14, 2025

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.0.md

@T-Gro
Copy link
Member

T-Gro commented Oct 15, 2025

Can I ignore the other three?
This one is fully green now.

@majocha
Copy link
Contributor Author

majocha commented Oct 15, 2025

Can I ignore the other three? This one is fully green now.

Yes, we can continue with just this one.

@majocha
Copy link
Contributor Author

majocha commented Oct 15, 2025

System.Exception : EVIL_PROVIDER_NamespaceName_Exception.err EVIL_PROVIDER_NamespaceName_Exception.bsl differ; "diff between [D:\a_work\1\s\artifacts\Temp\FSharp.Test.Utilities\72306301\6278ff13\typeProviders\negTests\EVIL_PROVIDER_NamespaceName_Exception.bsl] and [D:\a_work\1\s\artifacts\Temp\FSharp.Test.Utilities\72306301\6278ff13\typeProviders\negTests\EVIL_PROVIDER_NamespaceName_Exception.err]
diff at line 4
+



Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.

at [email protected](TcImportsLockToken tcitok)

at Internal.Utilities.Library.Lock1.AcquireLock[a](FSharpFunc2 f)

at FSharp.Compiler.CompilerImports.TcImportsSafeDisposal.Finalize()
"

Type providers don't like parallel imports, another random fail in evil provider.

@T-Gro
Copy link
Member

T-Gro commented Oct 16, 2025

Type providers don't like parallel imports, another random fail in evil provider.

I wonder if it because of the evil provider, or if the compiler should not synchronizing access to type provider code for all of them. (I assume that at time of TP creation, nothing in the compiler was parallel and access to TP libraries was always sequential).

@majocha
Copy link
Contributor Author

majocha commented Oct 16, 2025

Type providers don't like parallel imports, another random fail in evil provider.

I wonder if it because of the evil provider, or if the compiler should not synchronizing access to type provider code for all of them. (I assume that at time of TP creation, nothing in the compiler was parallel and access to TP libraries was always sequential).

I suspect it was because of one missing lock 6b96083
There was a comment that this requires a lock, but the lock was not there.

@majocha
Copy link
Contributor Author

majocha commented Oct 16, 2025

Type providers don't like parallel imports, another random fail in evil provider.

I wonder if it because of the evil provider, or if the compiler should not synchronizing access to type provider code for all of them. (I assume that at time of TP creation, nothing in the compiler was parallel and access to TP libraries was always sequential).

Yes, the oldest thing I remember was the Reactor, that made everything sequential. But then there's this #10310

@T-Gro
Copy link
Member

T-Gro commented Oct 16, 2025

Ok this makes sense, all access to TP synchronized.
We cannot tell from the outside if TP internals are thread safe or not.

@majocha majocha marked this pull request as ready for review October 16, 2025 18:55
@majocha majocha requested a review from a team as a code owner October 16, 2025 18:55

neg14a.fs(9,6,9,33): typecheck error FS0343: The type 'missingInterfaceInSignature' implements 'System.IComparable' explicitly but provides no corresponding override for 'Object.Equals'. An implementation of 'Object.Equals' has been automatically provided, implemented via 'System.IComparable'. Consider implementing the override 'Object.Equals' explicitly

neg14a.fs(2,8,2,11): typecheck error FS0193: Module 'Lib' requires a type 'z'
Copy link
Member

Choose a reason for hiding this comment

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

Those are legit - the signature has them, and implementation does not.

sigFiles
|> Array.map (fun sigFile ->
implFiles
|> Array.tryFind (fun (implFile: FileInProject) -> $"{implFile.FileName}i" = sigFile.FileName)
Copy link
Member

Choose a reason for hiding this comment

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

Luckily all of this is a no-op of sigFiles is empty (= most projects).

It does O(#sigFiles * #implFiles) new string allocations (for the interpolated string).

Do we need to be that liberate for the order?
I would have assumed that a sigFile can just look at idx+1, idx-1 in the overall files array;; and then compare spans of characters. Or are there test cases with more relaxed rules on the ordering?

We should aim to be

  • no-op if no sig files
  • linear if sig files present but correctly ordered
  • and only worse for degenerate orderings.

(for regular compilation it does not matter, but design time scenario via transparent compiler is IIRC making use of the graph)

Copy link
Contributor Author

@majocha majocha Oct 17, 2025

Choose a reason for hiding this comment

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

Yes, I'm not sure why it was like this instead of just comparing QualifiedNameOfFile.Id.

But this covers one edge case I was not aware of: fsi file does not need to directly precede the implementation, it just needs to come before. I can't remember out of my head but there are some public projects in the wild that do it that way.

EDIT because QualifiedNameOfFile is useless here.

I think Transparent Compiler caches the graphs, too. So the impact is not that bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants