Fixing duplicate intrinsics #880
Conversation
This fixes an issue where `body intrinsic` callables would appear twice in monomorphized results by pruning the list of concretized callables based on intrinsic settings.
|
@ScottCarda-MS I could use some tips on testing this change. I was able to manually verify from a built package that it eliminates the issue, but perhaps there are tests I can add in the runtime repo once this is merged? |
bettinaheim
left a comment
bettinaheim
left a comment
There was a problem hiding this comment.
Ah, thanks for finding this!
| var filter = new ResolveGenerics(callables.ToLookup(res => res.FullName.Namespace), intrinsicCallableSet, keepAllIntrinsics); | ||
| var filter = new ResolveGenerics( | ||
| callables | ||
| .Where(call => !keepAllIntrinsics || !intrinsicCallableSet.Contains(call.FullName)) |
There was a problem hiding this comment.
You could pull this out of the iteration.
|
@swernli For testing, there are some Monomorphization test cases found here: https://github.com/microsoft/qsharp-compiler/blob/main/src/QsCompiler/Tests.Compiler/TestCases/LinkingTests/Monomorphization.qs You could add a test case that defines a custom intrinsic, and then uses this intrinsic in the entry point. The testing logic would then verify that there is exactly the expected operations in the syntax tree after Monomorphization is run (it is run for all such test cases). I believe even just running a Signatures test on this would have that effect. We can discuss that more offline if you want to know more about how to do that. |
|
@ScottCarda-MS Thanks, that's perfect! Don't know how I missed that in my searches... |
This fixes an issue where
body intrinsiccallables would appear twice in monomorphized results by pruning the list of concretized callables based on intrinsic settings.