Disable deprecation warnings in deprecated callables and types

[msoeken]

Is your feature request related to a problem? Please describe. Deprecation warnings are raised for code used in deprecated functions or operations, even when they are never used.

Describe the solution you'd like I would like no warning to be raised in this case.

Tasks

• [x] Suppress deprecation warning in deprecated callables (#687)
• [ ] Suppress deprecation warning in deprecated types

[bettinaheim]

Agreed, this would be a good thing to do. This should be fairly easy to to - labeling it as a good first issue in case someone wants to pick it up.

[ziqi-ma]

Trying to work on this but not entirely sure what the problem is - is it producing warnings for deprecated methods in code that are never used? Correct me if I'm wrong - seems like deprecation is checked in SyntaxProcessor/SymbolTracker.fs function resolveGlobal. Currently a warning will be issued if it's an Operation or Function. Does this mean I should only check for deprecation for Operation not Function? Since Functions could be unused, and if they are used eventually they will be parsed as Operations?

[bamarsha]

Here is an example of the issue:

@Deprecated("")
function Foo() : Unit {
    Bar();
}

@Deprecated("")
function Bar() : Unit { }

A deprecation warning for Bar appears in Foo:

However, Foo itself is deprecated, so it would be reasonable if deprecated warnings were suppressed if the parent callable or type is itself deprecated (since you can assume the deprecated calls are only there for backwards compatibility until the callable is removed). This isn't related to whether Foo is an operation or function - it should work the same way in either case.

The same issue also exists for deprecated types, where the reference to Bar in Foo creates a deprecation warning:

@Deprecated("")
newtype Foo = Bar;

@Deprecated("")
newtype Bar = Unit;

Let me take a look at the code to see what the best way is to implement this, and I'll get back to you - @bettinaheim may also know more here.

[bamarsha]

GenerateDeprecationWarning is the source of the warning:

https://github.com/microsoft/qsharp-compiler/blob/e5468d610328bcc11e1ab9b354529caad9aa13a0/src/QsCompiler/Core/SymbolResolution.fs#L185-L190

I think we will need to tell GenerateDeprecationWarning if the parent callable/type is deprecated, so it can decide not to generate a warning. It's called twice - once for callables and once for types. For callables, it's in buildCallable in SymbolTracker.ResolveIdentifier:

https://github.com/microsoft/qsharp-compiler/blob/e5468d610328bcc11e1ab9b354529caad9aa13a0/src/QsCompiler/SyntaxProcessor/ScopeTools.fs#L207-L208

SymbolTracker knows the name of the parent callable, but not its attribute list (which is where the Deprecated attribute would be), although those are in the info variable here:

https://github.com/microsoft/qsharp-compiler/blob/e5468d610328bcc11e1ab9b354529caad9aa13a0/src/QsCompiler/CompilationManager/TypeChecking.cs#L1612-L1616

For types, it's called in tryResolveTypeName in NamespaceManager:

https://github.com/microsoft/qsharp-compiler/blob/e5468d610328bcc11e1ab9b354529caad9aa13a0/src/QsCompiler/Core/SymbolTable.fs#L848-L851

tryResolveTypeName is given the parent namespace, but not the attributes, as well - it's called in a couple places, but they don't seem to have direct access to the attributes either. It will probably need to look up the qualified name of the parent in the NamespaceManager to find its attributes. This looks trickier than the callable case, so I think I would recommend starting with that one first.

[ziqi-ma]

Thanks for the clarification! It's very helpful since I'm new to this codebase... I'm thinking about rewriting BuildCallable (in SymbolTracker.ResolveIdentifier, qsharp-compiler/src/QsCompiler/SyntaxProcessor/ScopeTools.fs line 207) as this:

let buildCallable kind fullName (decl : ResolvedSignature) attributes =
            // if parent is deprecated, no longer generate warning
            let parentAttrs = 
                match GlobalSymbols().TryGetCallable parent (parent.Namespace, sourceFile) with 
                | Found decl -> decl.Attributes
                | _ -> ArgumentException "the given NamespaceManager does not contain a callable with the given parent name" |> raise
            let deprecated =
                if not (Array.Exists(parentAttrs, BuiltIn.MarksDeprecation)) then Some (attributes |> SymbolResolution.TryFindRedirect |> SymbolResolution.GenerateDeprecationWarning (fullName, qsSym.RangeOrDefault))
                else None
            match deprecated with
            | Some dep -> for msg in dep do msg |> addDiagnostic
            | _ -> ()
            let argType, returnType = decl.ArgumentType |> StripPositionInfo.Apply, decl.ReturnType |> StripPositionInfo.Apply
            let idType = kind ((argType, returnType), decl.Information) |> ResolvedType.New 
            LocalVariableDeclaration.New false (defaultLoc, GlobalCallable fullName, idType, false), decl.TypeParameters

I'm not pulling the parent attributes from symbolTracker since it doesn't provide a hook to the callableDeclarations. I used GlobalSymbols similar to line 73 of ScopeTools.fs

I'm not sure whether checking BuiltIn.MarksDeprecation is sufficient, since I also see there are checks for unresolved attributes. In general I'm a little confused about what the attributes are and how they are resolved.

[bamarsha]

I'm not pulling the parent attributes from symbolTracker since it doesn't provide a hook to the callableDeclarations. I used GlobalSymbols similar to line 73 of ScopeTools.fs

That would work too, yeah. My thinking was that since the CallableDeclarationHeader was already found earlier, we could avoid needing to look it up again and handling the not-found case (even though we know it should be there) by passing it to SymbolTracker.

I'm not sure whether checking BuiltIn.MarksDeprecation is sufficient, since I also see there are checks for unresolved attributes. In general I'm a little confused about what the attributes are and how they are resolved.

I think as long as the types match, it should work. QsDeclarationAttribute should be the resolved version (the unresolved version is the DeclarationAttribute union case of QsFragmentKind, I think).

[ziqi-ma]

Thanks,

if not (Array.Exists(parentAttrs, BuiltIn.MarksDeprecation)) This is causing error but I'm not sure how to re-write - seems like it's expecting array [] but parentAttrs is ImmutableArray? Should I use a different "exists" method?

What are examples of a resolved vs. unresolved attribute?

Also, how should I test this?

[bamarsha]

if not (Array.Exists(parentAttrs, BuiltIn.MarksDeprecation)) This is causing error but I'm not sure how to re-write - seems like it's expecting array [] but parentAttrs is ImmutableArray? Should I use a different "exists" method?

Array.exists only works with the built-in array type, but you should be able to use Seq.exists which works with any sequence type.

What are examples of a resolved vs. unresolved attribute?

They both come from the same Q# source code, but are in different stages of compilation. In the first stage, only the name that was typed in the source code is known. After the name is looked up in the symbol table, it's resolved to a specific attribute defined somewhere else, and that's also when the compiler learns the type of the attribute and can check that it was used correctly. I think by the stage of the compilation that we're changing for this issue, all the attributes should be resolved.

Also, how should I test this?

There are tests for deprecation warnings here:

https://github.com/microsoft/qsharp-compiler/blob/a91f1214c630fe9f4651d5c20d18c1949abcdced/src/QsCompiler/Tests.Compiler/LocalVerificationTests.fs#L241-L242 https://github.com/microsoft/qsharp-compiler/blob/a91f1214c630fe9f4651d5c20d18c1949abcdced/src/QsCompiler/Tests.Compiler/TestCases/LocalVerification.qs#L1100-L1104

I think you can add some tests there to make sure no warnings are generated when the parent callable/type is itself deprecated.

[ziqi-ma]

Sent in a PR to address nested callable, now working on nested deprecated type:

check I understand correctly: in the previous example

// parent namespace @deprecated("") newtype foo = Unit;

@deprecated("") newtype bar = foo; // I'm assuming, after equal sign it starts the child namespace?

Currently what it does is: in function tryResolveTypeName (parentNS, source) ((nsName, symName), symRange : QsNullable), it checks whether symName (foo) is deprecated in parent namespace, which is True, thus it throws a warning. What we want: first check whether bar is deprecated. If so, don't need to check foo. To do that, basically just need to get the attributes of the header of "bar" in the parent namespace. And the function checkQualificationForDeprecation (line 133, NamespaceManager.fs) basically checks whether a symbol is deprecated in parent namespace, so if I can find the symbol bar I should just plug it in checkQualificationForDeprecation? But the problem is I don't know how to get the symbol "bar" - is it simply the child namespace name nsName?

[bamarsha]

newtype bar = foo; // I'm assuming, after equal sign it starts the child namespace?

This does not start a new namespace, actually - it's still part of the same namespace that bar is part of.

What we want: first check whether bar is deprecated. If so, don't need to check foo. To do that, basically just need to get the attributes of the header of "bar" in the parent namespace.

Yes, this sounds right.

And the function checkQualificationForDeprecation (line 133, NamespaceManager.fs) basically checks whether a symbol is deprecated in parent namespace, so if I can find the symbol bar I should just plug it in checkQualificationForDeprecation?

I think checkQualificationForDeprecation is for determining whether an attribute is the Deprecated attribute itself, rather than checking if a type is deprecated. I would put the check in the success function on line 136 of NamespaceManager.fs

But the problem is I don't know how to get the symbol "bar" - is it simply the child namespace name nsName?

It looks like nsName is actually the namespace of the symbol being resolved - so here it should be None, since foo is not a qualified symbol. I think tryResolveTypeName does not have enough information right now - I would try changing parentNS : NonNullable<string> to be parent : QsQualifiedName, so that you know both the parent namespace and the parent callable/type name.