support deprecated attribute in type constructors #3715

Wilbert-mad · 2024-10-19T00:12:14Z

Addressing: #3698

Analysis should report error when a custom types' variants are all deprecated
Parser should error if an unsupported attribute is used on a variant
LSP warning for deprecated variants
Formator allow attributes for type constructors

compiler-core/src/parse.rs

giacomocavalieri

This is looking nice! I left a couple of comments inline

compiler-core/src/parse.rs

giacomocavalieri · 2024-10-19T19:55:38Z

compiler-core/src/parse/error.rs

+            ParseErrorType::UnknownAttributeRecordConstructor => (
+                "Variant attribute(s) are unknow.",
+                vec!["Try `@deprecated`.".into()],
+            ),


I think I'd rather have an error that explicitly says that the attribute cannot be used on a variant for each of the attributes rather than a single error for all of those.

pub type Wibble { @external(javascript, "", "") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This attribute cannot be used on a type variant Wobble }

Also I would not include that hint, it doesn't make much sense to suggest using deprecated if I wrote an @external or @internal attribute

...rc/parse/snapshots/gleam_core__parse__tests__deprecation_attribute_on_all_type_variants.snap

lpil · 2024-10-20T11:26:26Z

Thank both!!

lpil · 2024-10-21T13:23:26Z

When you're ready for review please un-draft this PR 💜

compiler-core/src/format.rs

lpil · 2024-10-24T11:15:23Z

Is this ready for a review? :)

Wilbert-mad · 2024-10-24T12:05:51Z

Yes! :D

compiler-core/src/error.rs

CHANGELOG.md

GearsDatapacks · 2024-10-25T07:36:52Z

Just so you know, merge commits are not allowed in this repository. You'll need to rebase before Louis will accept it

Wilbert-mad · 2024-10-26T01:15:39Z

Alright, that should be good. Sorry about that. I'll leave it up to y'all from here.

giacomocavalieri · 2024-10-26T12:46:46Z

One thing is missing is version tracking. If someone is using the deprecated attribute on a constructor we want to make sure the version in their gleam.toml is >= 1.6 (if this gets merged before the 1.6 release).

This would require adding a new variant ConstructorWithDeprecatedAnnotation to the FeatureKind enum (

gleam/compiler-core/src/type_/error.rs

Lines 834 to 842 in 9ee71a1

    
           pub enum FeatureKind { 
        
               LabelShorthandSyntax, 
        
               ConstantStringConcatenation, 
        
               ArithmeticInGuards, 
        
               UnannotatedUtf8StringSegment, 
        
               NestedTupleAccess, 
        
               InternalAnnotation, 
        
               AtInJavascriptModules, 
        
           }

) and track its usage during the analysis phase I think

Wilbert-mad · 2024-10-27T02:12:26Z

Done!

lpil

Very nice!! This is nearly ready to go. I've left some small comments

CHANGELOG.md

compiler-core/src/analyse.rs

compiler-core/src/error.rs

compiler-core/src/parse.rs

compiler-core/src/type_/tests/custom_types.rs

Wilbert-mad · 2024-11-05T02:34:06Z

Finished!

lpil

Fab! I've left some notes inline improving the wording of messages and making internal types more accurate. After that we're ready to merge! Thank you

lpil · 2024-11-08T17:39:15Z

compiler-core/src/analyse.rs

@@ -878,6 +886,36 @@ impl<'a, A> ModuleAnalyzer<'a, A> {
            .parameters
            .clone();

+        // check if all constructors are deprecated if so error


Suggested change

// check if all constructors are deprecated if so error

// Check if all constructors are deprecated if so error.

lpil · 2024-11-08T17:39:21Z

compiler-core/src/analyse.rs

+                .error(Error::AllVariantsConstructorDeprecated { location });
+        }
+
+        // if any constructor record/varient is deprecated while


Suggested change

// if any constructor record/varient is deprecated while

// If any constructor record/varient is deprecated while

lpil · 2024-11-08T17:39:26Z

compiler-core/src/analyse.rs

+                .iter()
+                .any(|record| record.deprecation.is_deprecated())
+        {
+            // report error on all variants attibuted with deprecated


Suggested change

// report error on all variants attibuted with deprecated

// Report error on all variants attibuted with deprecated

lpil · 2024-11-08T17:40:02Z

compiler-core/src/error.rs

+  }
+");
+                            Diagnostic {
+                                title: "Deprecating all variants of a type is not allowed.".into(),


Suggested change

title: "Deprecating all variants of a type is not allowed.".into(),

title: "All variants of custom type deprecated.".into(),

lpil · 2024-11-08T17:40:21Z

compiler-core/src/error.rs

+                            let text = String::from("Consider removing the deprecation attribute on the type's variant.");
+
+                            Diagnostic {
+                                title: "Deprecating variants of a type that is deprecated is not allowed".into(),


Suggested change

title: "Deprecating variants of a type that is deprecated is not allowed".into(),

title: "Custom type already deprecated".into(),

lpil · 2024-11-08T17:42:29Z

compiler-core/src/parse/error.rs

@@ -398,6 +402,7 @@ pub enum ParseErrorType {
    ConstantRecordConstructorNoArguments, // const x = Record()
    TypeConstructorNoArguments,           // let a : Int()
    TypeDefinitionNoArguments,            // pub type Wibble() { ... }
+    UnknownAttributeRecordConstructor, // an attribute was used that is not know for a custom type constructor


Suggested change

UnknownAttributeRecordConstructor, // an attribute was used that is not know for a custom type constructor

UnknownAttributeRecordVariant // an attribute was used that is not know for a custom type variant

lpil · 2024-11-08T17:42:59Z

compiler-core/src/type_/error.rs

@@ -860,6 +880,7 @@ pub enum FeatureKind {
    AtInJavascriptModules,
    RecordUpdateVariantInference,
    RecordAccessVariantInference,
+    ConstructorWithDeprecatedAnnotation,


Suggested change

ConstructorWithDeprecatedAnnotation,

VariantWithDeprecatedAnnotation,

lpil · 2024-11-08T17:43:34Z

compiler-core/src/warning.rs

@@ -1033,6 +1033,9 @@ See: https://tour.gleam.run/functions/pipelines/",
                        FeatureKind::RecordAccessVariantInference => {
                            "Field access on custom types when the variant is known was"
                        }
+                        FeatureKind::ConstructorWithDeprecatedAnnotation => {
+                            "The constructor's `@deprecated` annotation was"


Suggested change

"The constructor's `@deprecated` annotation was"

"Deprecating individual custom type variants was"

lpil · 2024-11-08T17:43:52Z

compiler-core/src/type_/error.rs

@@ -972,7 +994,9 @@ impl Error {
            }
            | Error::UseFnDoesntTakeCallback { location, .. }
            | Error::UseFnIncorrectArity { location, .. }
-            | Error::BadName { location, .. } => location.start,
+            | Error::BadName { location, .. }
+            | Error::AllVariantsConstructorDeprecated { location }


Suggested change

| Error::AllVariantsConstructorDeprecated { location }

| Error::AllVariantsDeprecated { location }

lpil · 2024-11-08T17:44:12Z

compiler-core/src/type_/error.rs

-            | Error::BadName { location, .. } => location.start,
+            | Error::BadName { location, .. }
+            | Error::AllVariantsConstructorDeprecated { location }
+            | Error::VariantDeprecatedOnDeprecatedConstructor { location } => location.start,


Suggested change

| Error::VariantDeprecatedOnDeprecatedConstructor { location } => location.start,

| Error::DeprecatedVariantOnDeprecatedType { location } => location.start,

lpil · 2024-11-10T12:35:54Z

Hi @Wilbert-mad, you've force pushed to an older version of the code so there's now merge conflicts. Please rebase on main. Thank you

compiler-core/src/type_/error.rs

Wilbert-mad commented Oct 19, 2024

View reviewed changes

compiler-core/src/parse.rs Outdated Show resolved Hide resolved

giacomocavalieri reviewed Oct 19, 2024

View reviewed changes

lpil marked this pull request as draft October 20, 2024 11:26

Wilbert-mad commented Oct 23, 2024

View reviewed changes

compiler-core/src/format.rs Show resolved Hide resolved

Wilbert-mad marked this pull request as ready for review October 23, 2024 23:41

GearsDatapacks reviewed Oct 24, 2024

View reviewed changes

compiler-core/src/error.rs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

Wilbert-mad force-pushed the deprecated-tag-in-type-constructor branch from 9857f7c to 836d5a4 Compare October 26, 2024 00:55

Wilbert-mad force-pushed the deprecated-tag-in-type-constructor branch from 5a06896 to 3828be4 Compare October 27, 2024 01:02

lpil reviewed Nov 2, 2024

View reviewed changes

lpil force-pushed the deprecated-tag-in-type-constructor branch 3 times, most recently from 84845bb to 33f9f03 Compare November 7, 2024 16:01

lpil reviewed Nov 8, 2024

View reviewed changes

Wilbert-mad force-pushed the deprecated-tag-in-type-constructor branch from c1908bb to ec69f60 Compare November 9, 2024 17:49

Wilbert-mad added 6 commits November 10, 2024 16:36

rebase fix

31c7d83

fix typo

88f5a81

fix typo

f1a9f7b

fix incorrection

231f4ae

more testing with attrs in variants and error message

dc65c5d

shorter error span

185f409

Wilbert-mad added 14 commits November 10, 2024 16:40

all variants error in analyse phase

b1d5edc

remove removed snapshot test in parser that was moved to analysis

cf4dcbc

adjust formater

b13cc8f

fix formatting

110e01d

lsp & cleanup

292e948

tests

709ef77

version tracking

03dd501

suggested fixes

cc9f638

fix

62671ab

fix

d1b7f14

fix comments

be82f56

fix: suggested fixes

035dbbf

fix

c12466c

fix

f8280b9

Wilbert-mad force-pushed the deprecated-tag-in-type-constructor branch from ec69f60 to f8280b9 Compare November 11, 2024 04:39

giacomocavalieri reviewed Nov 11, 2024

View reviewed changes

compiler-core/src/type_/error.rs Outdated Show resolved Hide resolved

feature increase version

1a2407f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support deprecated attribute in type constructors #3715

support deprecated attribute in type constructors #3715

Wilbert-mad commented Oct 19, 2024 •

edited

Loading

giacomocavalieri left a comment

giacomocavalieri Oct 19, 2024

lpil commented Oct 20, 2024

lpil commented Oct 21, 2024

lpil commented Oct 24, 2024

Wilbert-mad commented Oct 24, 2024

GearsDatapacks commented Oct 25, 2024

Wilbert-mad commented Oct 26, 2024

giacomocavalieri commented Oct 26, 2024 •

edited

Loading

Wilbert-mad commented Oct 27, 2024

lpil left a comment •

edited

Loading

Wilbert-mad commented Nov 5, 2024

lpil left a comment

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil Nov 8, 2024

lpil commented Nov 10, 2024 •

edited

Loading

	// check if all constructors are deprecated if so error
	// Check if all constructors are deprecated if so error.

	// if any constructor record/varient is deprecated while
	// If any constructor record/varient is deprecated while

	// report error on all variants attibuted with deprecated
	// Report error on all variants attibuted with deprecated

	title: "Deprecating all variants of a type is not allowed.".into(),
	title: "All variants of custom type deprecated.".into(),

	title: "Deprecating variants of a type that is deprecated is not allowed".into(),
	title: "Custom type already deprecated".into(),

	UnknownAttributeRecordConstructor, // an attribute was used that is not know for a custom type constructor
	UnknownAttributeRecordVariant // an attribute was used that is not know for a custom type variant

	ConstructorWithDeprecatedAnnotation,
	VariantWithDeprecatedAnnotation,

	"The constructor's `@deprecated` annotation was"
	"Deprecating individual custom type variants was"

	\| Error::AllVariantsConstructorDeprecated { location }
	\| Error::AllVariantsDeprecated { location }

	\| Error::VariantDeprecatedOnDeprecatedConstructor { location } => location.start,
	\| Error::DeprecatedVariantOnDeprecatedType { location } => location.start,

support deprecated attribute in type constructors #3715

Are you sure you want to change the base?

support deprecated attribute in type constructors #3715

Conversation

Wilbert-mad commented Oct 19, 2024 • edited Loading

giacomocavalieri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil commented Oct 20, 2024

lpil commented Oct 21, 2024

lpil commented Oct 24, 2024

Wilbert-mad commented Oct 24, 2024

GearsDatapacks commented Oct 25, 2024

Wilbert-mad commented Oct 26, 2024

giacomocavalieri commented Oct 26, 2024 • edited Loading

Wilbert-mad commented Oct 27, 2024

lpil left a comment • edited Loading

Choose a reason for hiding this comment

Wilbert-mad commented Nov 5, 2024

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil commented Nov 10, 2024 • edited Loading

Wilbert-mad commented Oct 19, 2024 •

edited

Loading

giacomocavalieri commented Oct 26, 2024 •

edited

Loading

lpil left a comment •

edited

Loading

lpil commented Nov 10, 2024 •

edited

Loading