Add `Deprecated` and `Unimplemented` attributes #785

swernli · 2023-10-12T00:08:45Z

This change adds support for two new attributes:

Deprecated which will produce a warning if the attributed callable is used and can accept either unit/() or a callable expression as the alternative. If an expression is provided, it will be name resolved and type checked.
Unimplemented which produces an error if the attributed callalbe is used and only accepts unit/().

The idea is to be able to use these to indicate parts of the standard library are either getting ready to be removed or entirely not present. The expectationsi that they could be used like this (using two old Bitwise utilities):

@Deprecated((a, b) -> a ^^^ b)
function Xor(a : Int, b : Int) : Int {
    return a ^^^ b;
}

@Unimplemented()
function XBits(paulis : Pauli[]) : Int {
    fail "unimplemented";
}

And here is how that would show up if user code called these functions:

compiler/qsc_frontend/src/resolve.rs

minestarks

Left some comments. Code looks good but I have reservations about the design and the feature in general.

minestarks · 2023-10-25T17:29:59Z

compiler/qsc_hir/src/hir.rs

+    /// Indicates that an item has been deprecated and may eventually be removed.
+    Deprecated(Option<Span>),
+    /// Indicates that an item does not have an implementation available for use.
+    Unimplemented,


I'm not really sold on having two separate attributes. Is Unimplemented like a more severe Deprecated? Or is it for things that we literally can't implement, where the implementation would just have a fail statement or something? In either case, I feel like we could just reuse the Deprecated attribute, perhaps with an additional flag or comment.

Deprecated is typically something implemented but doomed to be removed soon.

minestarks · 2023-10-25T17:30:35Z

compiler/qsc_hir/src/hir.rs

    /// Indicates that a callable is an entry point to a program.
    EntryPoint,
+    /// Indicates that an item has been deprecated and may eventually be removed.
+    Deprecated(Option<Span>),


I think it would be nice if this could take a user provided message (which would then be displayed in the warning).

minestarks · 2023-10-25T17:32:15Z

compiler/qsc_hir/src/hir.rs

@@ -1126,8 +1155,28 @@ impl Display for Ident {
 /// An attribute.
 #[derive(Clone, Debug, PartialEq)]
 pub enum Attr {
+    /// Provide pre-processing information about when an item should be included in compilation.
+    Config,


Just a thought, not sure if I love it or not: Could we overload the @Config attribute for the purposes of deprecation? Like put some items behind a configuration that will never exist in practice (like None or literally Deprecated). The error message could then provide a little more context based on the configuration, or the attribute could take a user-provided message.

minestarks · 2023-10-25T18:11:58Z

compiler/qsc_frontend/src/typeck/check.rs

@@ -175,6 +176,16 @@ impl Checker {
        ));
    }

+    fn check_attr_expr(&mut self, names: &Names, expr: &Expr) {


Honestly it feels weird to allow an expression here. Can we just allow a Path to a global if the intention is "use this alternative API"? Why would we allow lambdas? If we're allowing expressions, why not statements too? I don't think I get the use case you have in mind.

minestarks · 2023-10-25T18:17:23Z

compiler/qsc_frontend/src/resolve.rs

+    ))]
+    #[diagnostic(severity(Warning))]
+    #[diagnostic(code("Qsc.Resolve.Deprecated"))]
+    Deprecated(String, #[label] Span),


I feel iffy about having this baked into the resolver, I feel like it could be a separate pass, both for future-facing reasons (such as the ability to disable and order checks, as we talked about), but also because it feels like scope creep for the core parts of the compiler.

swernli · 2023-10-25T21:13:24Z

@minestarks is asking some great questions, and even pointed out that the current approach doesn't fully update for the existence of a new warning type. For example, the playground and QCOM disable the run button if any errors are present, but check that by just looking at whether the length of VSDiagnostics returned from compilation is greater than zero. We can improve this via filtering (and should) but it might be worth a more holistic look at what a warning means and how it should be surfaced.

I'm changing this to draft to give us the chance to work through these questions and decide if a separate, "prepare for warnings" PR is needed in the infrastructure. I think it might be worth reviewing that on it's own separate from the feature being introduced here.

swernli · 2023-10-28T17:38:04Z

Ok, after thinking about it some more, I have an approach I'd like to try. I'm going to split this up into two PRs.

The first PR will introduce the Unimplemented attribute and include the refactoring of attribute handling to be enum-based as well as the error reporting for the new Unimplemented attribute. There, we can focus on deciding whether that error makes sense in name resolution or should be pulled out to a pass and make sure the infrastructure for annotating references with relevant attribute info is acceptable.

Then the second, follow-up PR can introduce the Deprecated attribute, which will be the first warning in the new stack. That PR discussion can focus on making sure we agree on how warnings are handled throughout the stack and tools, as well as making sure Deprecated itself is handled in a consistent way to give a good use experience.

swernli · 2023-10-30T22:58:17Z

part 1 is now up as #826. Closing this in favor of that approach.

swernli added 5 commits October 17, 2023 15:45

WIP: add Deprecated, Unimplemented attrs

d13455c

Allow empty deprecation attr

b184e32

Update language service

10076df

Avoid ambiguous error on deprecated/unimplemented

6b177a3

Add tests

51c4514

swernli force-pushed the swernli/impl-attrs branch from 092a967 to 51c4514 Compare October 17, 2023 22:45

swernli added 3 commits October 17, 2023 20:50

Rework item info tracking as item status

f2b36ba

Move error reporting up in stack

fdf2599

Disallow unimplemented within unit

8fb97f7

swernli marked this pull request as ready for review October 18, 2023 04:26

swernli requested review from idavis and minestarks as code owners October 18, 2023 04:26

sezna reviewed Oct 19, 2023

View reviewed changes

compiler/qsc_frontend/src/resolve.rs Show resolved Hide resolved

sezna approved these changes Oct 23, 2023

View reviewed changes

minestarks reviewed Oct 25, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into swernli/impl-attrs

fad70ae

swernli marked this pull request as draft October 25, 2023 21:10

swernli closed this Oct 30, 2023

swernli mentioned this pull request Oct 31, 2023

Support Unimplemented attribute #826

Merged

swernli deleted the swernli/impl-attrs branch December 13, 2023 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Deprecated` and `Unimplemented` attributes #785

Add `Deprecated` and `Unimplemented` attributes #785

swernli commented Oct 12, 2023 •

edited

Loading

minestarks left a comment

minestarks Oct 25, 2023

DmitryVasilevsky Nov 2, 2023

minestarks Oct 25, 2023

minestarks Oct 25, 2023

minestarks Oct 25, 2023

minestarks Oct 25, 2023

swernli commented Oct 25, 2023

swernli commented Oct 28, 2023

swernli commented Oct 30, 2023

Add Deprecated and Unimplemented attributes #785

Add Deprecated and Unimplemented attributes #785

Conversation

swernli commented Oct 12, 2023 • edited Loading

minestarks left a comment

Choose a reason for hiding this comment

minestarks Oct 25, 2023

Choose a reason for hiding this comment

DmitryVasilevsky Nov 2, 2023

Choose a reason for hiding this comment

minestarks Oct 25, 2023

Choose a reason for hiding this comment

minestarks Oct 25, 2023

Choose a reason for hiding this comment

minestarks Oct 25, 2023

Choose a reason for hiding this comment

minestarks Oct 25, 2023

Choose a reason for hiding this comment

swernli commented Oct 25, 2023

swernli commented Oct 28, 2023

swernli commented Oct 30, 2023

Add `Deprecated` and `Unimplemented` attributes #785

Add `Deprecated` and `Unimplemented` attributes #785

swernli commented Oct 12, 2023 •

edited

Loading