Apply common project properties to LlvmBinding project

[AndreyTretyak]

PR for the issue #922

I'm sorry in advance for the PR size. I should've stopped earlier :)
Most of the changes are automatic, I'll try to describe the most important parts here.
I also understand that it's a risky change to take from an external contributor, so feel free to decline.

StyleCop

Most of the changes are about adjusting whitespaces, reordering using's, adding this. and dots in comments, and reordering constructors (the most annoying part). Most of the changes that I consider safe are here f3589a6 and here 2bc853f

Cases where I actually had to change code:

1. Change naming for private fields to start with lower case letters. Since they were private I don't expect problems unless types are serialized or some kind of reflection involved. Commit with the change here: 4ce681d
2. I had to change the names of the tuple members returned by GlobalCopyAllMetadata, so now they start with an upper case letter. It might be a problem since the method is public, an alternative could be just suppressing warning. Commit with the change 84f83e0

-public static (LLVMValueMetadataEntryRef metadataRef, uint count) GlobalCopyAllMetadata(this LLVMValueRef self)
+public static (LLVMValueMetadataEntryRef MetadataRef, uint Count) GlobalCopyAllMetadata(this LLVMValueRef self)

3. BitcodeModule has two fields that StyleCop really doesn't like.

• IsDisposed is a public field that basically allows anybody to set it to true, that looked wrong so I replaced it with property (public getter and private setter), but there might be a reason for it to be how it is.

-public bool IsDisposed;
+public bool IsDisposed { get; private set; }

• ModuleHandle is an internal property with a mutable struct as type, so I've replaces it with a private field and internal property that returns value by ref. It should provide the same behavior, excluding recursion.
  Commit with the change 84f83e0

-internal LLVMModuleRef ModuleHandle;
+private LLVMModuleRef moduleHandle;
+internal ref LLVMModuleRef ModuleHandle => ref this.moduleHandle;

Nullablity

The majority of the changes are about marking return type as nullable, so they should be only beneficial.
Most of them are here be2128a
But the are some risky changes where I've decided to change behavior:

1. I had to suppress working about nullable type in ValueAttributeDictionary.TryGetValue because in netstandard2.1 dictionary interface didn't have proper parameter attribute, suppression could be removed after project target changed to net5.0 or higher. It might cause incorrect nullable marking if the type used via the interface in projects targeting netcoreapp3.1 or lower, but that also the case for any dictionary.

-public bool TryGetValue(FunctionAttributeIndex key, out ICollection<AttributeValue> value)
+#pragma warning disable CS8767 // IReadOnlyDictionary<TKey,TValue> interface does not have nullability attributes in netstandard2.1, this suppression could be removed after move to .NET 5
+public bool TryGetValue(FunctionAttributeIndex key, [MaybeNullWhen(false)] out ICollection<AttributeValue> value)
+#pragma warning restore CS8767

2. The most risky changes of this PR are in Value.FromHandle<T> and TypeRef.FromHandle<T>, both of them used as for casting so could return null in case of the wrong type, but judging from methods usage null was not expected, so I assumed it's that replacing it with direct cast might be more appropriate. In addition to that TypeRef.FromHandle<T>(LLVMTypeRef typeRef) was returning null if input struct is default, so I've removed the check, so it would fail with ArrgumentException("Handle is null"). I understand it's very controversial decisions and behavior change, an alternative would mean marking a big chunk of the project with nullability markers and checks. That's definitely possible, but I've wanted to check with you first that it would be a proper thing to do here. Commit ea47021

public class Value
{
        internal static T FromHandle<T>(LLVMValueRef valueRef) where T : Value
        {
            var context = valueRef.GetContext();
-           return context.GetValueFor(valueRef) as T;
+           return (T)context.GetValueFor(valueRef);
        }
}
internal class TypeRef
{
        internal static T FromHandle<T>(LLVMTypeRef typeRef) where T : class, ITypeRef
        {
-           if( typeRef == default )
-           {
-               return default;
-           }

            var ctx = GetContextFor(typeRef);
-           return ctx.GetTypeFor(typeRef) as T;
+           return (T)ctx.GetTypeFor(typeRef);
        }
}

Future improvements

1. StyleCop suggests replacing the copyright header with the default one (SA1636), but I'm not sure if it's acceptable for this project.
2. Warnings about unknown types used in cref of documentation comments (CS1574) currently suppressed. I'm not sure where they lost during refactoring, were never accessible, or just require a proper namespace. Probably the easiest fix would be replacing cref with regular text if nobody knows how to reference them.
3. Project still has 19 suggestions about removing suppression that not needed (IDE0079). Should be a really easy change, but I didn't want to make PR bigger.
4. Import of common AssemblyCommon.props file could be done by putting import into Directory.Build.props in the root of the repo, then each new project would automatically use it without the need to modify the project (you can also add a way of disabling it if needed). More info https://docs.microsoft.com/en-us/visualstudio/msbuild/customize-your-build?view=vs-2019

[swernli]

Thanks @AndreyTretyak for the great contribution! Regarding the StyleCop suggestion for changing the header, that is indeed left intentionally to indicate the project these files were originally copied from. But all the rest of the style clean up sounds excellent. For the missing types, this is most likely because they have been intentionally removed as part of adapting the code to our use, but not all doc comment references were removed, and I agree that can be fixed in a later pass through the code. I should have time to review this over the next day or two, and will ping you if I have any questions or concerns.

[swernli]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[bamarsha]

Regarding the StyleCop suggestion for changing the header, that is indeed left intentionally to indicate the project these files were originally copied from.

It should be possible to have a different StyleCop configuration for the LlvmBindings project that specifies the right copyright header.

[swernli]

Lot's here, but it looks good! Just a couple minor comments that you can either address now or can be cleaned up later. Thanks again!

[bettinaheim]

Awesome! Thanks, @AndreyTretyak for the great contribution!

[AndreyTretyak]

Regarding the StyleCop suggestion for changing the header, that is indeed left intentionally to indicate the project these files were originally copied from.

It should be possible to have a different StyleCop configuration for the LlvmBindings project that specifies the right copyright header.

Nice idea. I've tested it and it works and even it even located some wrong headers but raises some additional questions (ex. config override, common props changes, and header in delay sign), so I would prefer to submit it as a follow-up to simplify the discussion.

[AndreyTretyak]

@swernli thank you for the review, I can imagine it was not easy.
I've pushed small fixes and if you don't mind I would prefer to complete this PR and make mentioned improvements in the follow-up (separate stylecop.json, common config improvements, removing warnings suppression).
So feel free to merge if you are okay with it.

[swernli]

@AndreyTretyak Sounds good to me. I set up the auto-merge, so assuming the latest merge from main and build succeeds it should go in. Thanks again!

[AndreyTretyak]

/azp list

[azure-pipelines]

Commenter does not have sufficient privileges for PR 931 in repo microsoft/qsharp-compiler

[AndreyTretyak]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 931 in repo microsoft/qsharp-compiler

[AndreyTretyak]

Commenter does not have sufficient privileges for PR 931 in repo microsoft/qsharp-compiler

@swernli I think I need your help triggering the last validation pipeline

[swernli]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[AndreyTretyak]

@swernli @SamarSha I'm sorry for pinging you again.
It looks like the build started failing after the last update from main. Is it possible or could it be intermittent failure?
If it's not the case could you help me triaging failure? I doubt that it's the latest commit e29768b, might be something before it.

FAILED: lib/QSharpFoundation/Microsoft.Quantum.Qir.QSharp.Foundation.dll lib/QSharpFoundation/Microsoft.Quantum.Qir.QSharp.Foundation.lib 
cmd.exe /C "cd . && C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE -fuse-ld=lld-link -nostartfiles -nostdlib -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -O3 -DNDEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrt   -shared -o lib\QSharpFoundation\Microsoft.Quantum.Qir.QSharp.Foundation.dll  -Xlinker /implib:lib\QSharpFoundation\Microsoft.Quantum.Qir.QSharp.Foundation.lib -Xlinker /pdb:lib\QSharpFoundation\Microsoft.Quantum.Qir.QSharp.Foundation.pdb -Xlinker /version:0.0 lib/QSharpFoundation/qsharp-foundation-qis.obj lib/QSharpFoundation/CMakeFiles/qsharp-foundation-qis-support-obj.dir/intrinsicsMath.cpp.obj lib/QSharpFoundation/CMakeFiles/qsharp-foundation-qis-support-obj.dir/conditionals.cpp.obj  -LD:/a/1/s/submodules/qsharp-runtime/src/QirRuntime/build/Release/lib/QIR  -lMicrosoft.Quantum.Qir.Runtime  -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -loldnames && cd ."
lld-link: error: could not open 'Microsoft.Quantum.Qir.Runtime.lib': no such file or directory
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

Could it be related to this PR somehow?

[swernli]

It does appear to have been an intermittent failure, since it passed on rerun, but it's not one I've seen before, so I'll keep an eye out in case it crops up again.