Add annotation printer #3007

omprakaash · 2024-04-18T16:10:05Z

Adds an annotation printer class that is to be used for inlay hints/code annotations in the language server. This is similar to the pretty_printer, but also context aware.
Ref: #2525

omprakaash · 2024-04-20T04:23:26Z

compiler-core/src/language_server/annotation_printer.rs

+
+impl<'a> AnnotationPrinter<'a> {
+    pub fn new(
+        type_aliases: &'a HashMap<(EcoString, EcoString), EcoString>,


Would it be better if we make the constructor just take in the module as an argument ? We could then construct the necessary context in the constructor.

Could be good, however I think that would make it a lot harder to write the tests as there would be a great deal more information that would be needed to construct this class.

lpil

Wicked! Thank you. I've left a handful of comments inline. Let me know if anything is unclear.

lpil · 2024-04-20T13:55:53Z

compiler-core/src/language_server/annotation_printer.rs

+    }
+
+    pub fn print(&mut self, typ: &Type) -> String {
+        let mut typ_str = String::new();


We can use an ecostring here!

Oh yes, will change the buffer to an EcoString.

lpil · 2024-04-20T13:57:05Z

compiler-core/src/language_server/annotation_printer.rs

+                let key = (module.clone(), name.clone());
+
+                if let Some(typ_alias) = self.type_aliases.get(&key) {
+                    // Type has been aliased. eg: import mod1.{type Cat as c} or type UserId = Int


Suggested change

// Type has been aliased. eg: import mod1.{type Cat as c} or type UserId = Int

// Type has been aliased. eg: import mod1.{type Cat as C} or type UserId = Int

Typo!

lpil · 2024-04-20T13:57:54Z

compiler-core/src/language_server/annotation_printer.rs

+    // Mapping of module.type to type alias -
+    // eg: import mod1.{type Cat as c} -> Key: (mod1, Cat), Value: c
+    // eg: type UserId = Int -> Key: (mod, UserId), Value: Int


Sorry, bit confused by these. Could you use Rust syntax for the key values and explain with text what each example is showing. Thanks!

Oh the field type_aliases contains mapping of pairs of types and the module in which they are defined to their alias in the current module. (Sorry the second example is actually flipped since the value has to be UserId).

So for example, lets say there is an import statement in a module like so import mod1.{type Tiger as Tig}. This would result in a map entry with the key being ("mod1", "Tiger") and the value being Tig. -> type_aliases.insert(("mod1", "Tiger"), "Tig")

Another example is that of an type alias statement like so in a module named foo: type UserId = Int. Since Int has been aliased by UserId, an entry in the map is inserted, with they key being a pair of (foo, Int) and the value being UserId. (type_aliases.insert(("foo", "Int"), "UserId"). So any occurrences of the above types in a module which are not already annotated by the user, could be annotated with their aliased types.

I think that type alias one is going to break for generics

Hmm sorry, not quite sure on how this would break generics. Could you give me an example ?
Edit: This field does not deal with generics. The field type_parameters helps deal with them

lpil · 2024-04-20T13:58:20Z

compiler-core/src/language_server/annotation_printer.rs

+    // eg: type UserId = Int -> Key: (mod, UserId), Value: Int
+    type_aliases: &'a HashMap<(EcoString, EcoString), EcoString>,
+
+    // Mapping of module alias to module name


Could you give an example for this too

This is just used to keep track of module aliases.
eg: import testmod as test
So the key would be "testmod" and the vale "test"

lpil · 2024-04-20T13:58:26Z

compiler-core/src/language_server/annotation_printer.rs

+    type_parameters: &'a HashMap<u64, TypeAst>,
+    current_module: EcoString,


What are these?

Sry, type_parameters is probably not a clear name for this. What this does it keeps track of user annotations for generic variables in a function.
eg 1:

fn foo(arg1: gen, arg2){ arg1 == arg2 }

-> Here the annotation that will be generated for arg2 will be gen

eg2:

fn identity(x) { let z = x let y: value = x y }

The type annotations generated for x and z will be 'value'.

lpil · 2024-04-20T14:02:22Z

compiler-core/src/language_server/annotation_printer.rs

+                    }
+                    typ_str.push_str(name.as_str());
+                } else {
+                    // Always qualify types from other modules


If the type has been imported in an unqualified fashion then we want to use the unqualified constructor, not always qualify it.

Oh Yes. Will change this. Might have to keep track of all unqualified imports in the module then.

lpil · 2024-04-20T14:03:01Z

compiler-core/src/language_server/annotation_printer.rs

+        }
+    }
+
+    fn args_to_string(&mut self, args: &[Arc<Type>]) -> String {


Rather than build an extra string all methods should push to the existing string buffer

lpil · 2024-04-20T14:04:49Z

compiler-core/src/language_server/annotation_printer.rs

+        }
+    }
+
+    fn name_clashes_if_unqualified(&mut self, type_: &EcoString, module: &str) -> bool {


What does this function mean? I'm not sure what it is doing looking at the previously printed types.

I think it's here to work out if a prelude type has been shadowed by a type defined in this module, but you can't work that out by looking at what has been printed. Rather, we need to know the names of all the types that are in scope in this module, both defined and imported.

Yep. this is meant to check to check if a prelude type is shadowed. I see, yeah I think this depends on the order of printing being just right to figure out if it is shadowed. Will include info of all types in scope of the module to check for any shadowing.

lpil · 2024-04-20T14:06:19Z

compiler-core/src/language_server/annotation_printer.rs

+        }
+    }
+
+    pub fn print(&mut self, typ: &Type) -> String {


This method constructs a great many strings- instead let's have it push strs onto the mutable buffer so it only ever allocates one and then grows that as needed.

I'd do this by having a public method that returns a string, and internally it resets an buffer and then calls a private printing function which recursively calls itself and each call will append to the buffer.

Gotcha Thank you !

lpil · 2024-04-20T14:08:14Z

compiler-core/src/language_server/annotation_printer.rs

+    };
+
+    assert_eq!(printer.print(&typ), "T");
+}


Nice! Let's have tests for all the other possible paths through the type printer. We want full coverage to make sure that we are generating correct code.

Gotcha. Will add more tests.

Are you working on these? 🙏

omprakaash · 2024-04-27T10:01:09Z

Hey, I changed the logic for annotation generation based on the earlier review and added some comments as well. I am still a bit unclear on how the current use of type_aliases breaks generics (#3007 (comment)). Will add more test cases as soon that is fixed.

lpil · 2024-05-01T10:12:43Z

compiler-core/src/language_server/annotation_printer.rs

+        }
+    }
+
+    pub fn print_type(&mut self, typ: &Type) -> String {


We use EcoString, not String. If conversion is needed to interact with some library we should do it at the boundary of the system, not internally

lpil · 2024-05-01T10:19:20Z

compiler-core/src/language_server/annotation_printer.rs

+    let mut generic_annotations = HashMap::new();
+
+    let type_var = TypeAst::Var(crate::ast::TypeAstVar {
+        name: EcoString::from("T"),


This isn't valid Gleam, it should be lowercase for a variable

lpil · 2024-05-01T10:19:41Z

compiler-core/src/language_server/annotation_printer.rs

+    };
+
+    assert_eq!(printer.print(&typ), "T");
+}


Are you working on these? 🙏

lpil

Nice! Once we have all the tests we can merge this I think

lpil · 2024-05-08T11:09:01Z

compiler-core/src/language_server/annotation_printer.rs

+        package: EcoString::from(""),
+    };
+
+    assert_eq!(printer.print_type(&typ), "Animals.Cat");


This is not valid Gleam. What happened here?

raquentin · 2024-05-20T21:49:00Z

Is this stable? I can complete #3001 if so

lpil · 2024-05-24T21:20:28Z

Thank you!! Gunna use this as a dev branch while v1.2.0 is worked through.

omprakaash force-pushed the lsp-printer branch from dc2625b to 6f2ec65 Compare April 20, 2024 00:57

omprakaash changed the title ~~Add LSP Printer~~ Add annotation printer Apr 20, 2024

omprakaash force-pushed the lsp-printer branch from 6f2ec65 to 9afd26a Compare April 20, 2024 00:58

omprakaash commented Apr 20, 2024

View reviewed changes

lpil reviewed Apr 20, 2024

View reviewed changes

lpil mentioned this pull request Apr 22, 2024

Feat: add annotation LSP code action #3001

Closed

omprakaash force-pushed the lsp-printer branch from 9afd26a to a31131f Compare April 27, 2024 09:58

lpil reviewed May 1, 2024

View reviewed changes

lpil added this to the LS02 milestone May 3, 2024

omprakaash force-pushed the lsp-printer branch 2 times, most recently from 92186e3 to 2e295ee Compare May 4, 2024 15:41

lpil reviewed May 8, 2024

View reviewed changes

omprakaash force-pushed the lsp-printer branch from 2e295ee to 5479e74 Compare May 14, 2024 15:27

lpil force-pushed the lsp-printer branch 2 times, most recently from 83747b1 to 07a0cc5 Compare May 24, 2024 20:48

lpil self-assigned this May 24, 2024

lpil marked this pull request as draft May 24, 2024 21:19

omprakaash and others added 5 commits May 30, 2024 16:03

Add annotation printer in language server

a027a02

Minor Fixes and more unit tests

e4ef1f9

Remove unnecessary module prefix in type annotations and add more tests

66101a4

Clean up docs and names

0a7f514

Extract TypeNames struct

130597c

lpil force-pushed the lsp-printer branch from 07a0cc5 to 130597c Compare May 30, 2024 15:03

lpil added 4 commits May 30, 2024 16:33

Move type name finding to TypeNames

4598f41

Simplify interface

db3116f

Simplify API more

eaf0b26

Simplify, qualify types not in scope

4ce781e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add annotation printer #3007

Add annotation printer #3007

omprakaash commented Apr 18, 2024 •

edited

omprakaash Apr 20, 2024

lpil Apr 20, 2024

lpil left a comment

lpil Apr 20, 2024

omprakaash Apr 21, 2024

lpil Apr 20, 2024

lpil Apr 20, 2024

omprakaash Apr 22, 2024

lpil Apr 22, 2024

omprakaash Apr 22, 2024 •

edited

lpil Apr 20, 2024

omprakaash Apr 23, 2024

lpil Apr 20, 2024

omprakaash Apr 22, 2024

lpil Apr 20, 2024

omprakaash Apr 22, 2024

lpil Apr 20, 2024

lpil Apr 20, 2024

omprakaash Apr 22, 2024 •

edited

lpil Apr 20, 2024

omprakaash Apr 22, 2024

lpil Apr 20, 2024

omprakaash Apr 22, 2024

lpil May 1, 2024

omprakaash commented Apr 27, 2024

lpil May 1, 2024

lpil May 1, 2024

lpil May 1, 2024

lpil left a comment

lpil May 8, 2024

raquentin commented May 20, 2024

lpil commented May 24, 2024

	// Type has been aliased. eg: import mod1.{type Cat as c} or type UserId = Int
	// Type has been aliased. eg: import mod1.{type Cat as C} or type UserId = Int

		type_parameters: &'a HashMap<u64, TypeAst>,
		current_module: EcoString,

Add annotation printer #3007

Are you sure you want to change the base?

Add annotation printer #3007

Conversation

omprakaash commented Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

omprakaash Apr 22, 2024 • edited

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

omprakaash Apr 22, 2024 • edited

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

omprakaash commented Apr 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raquentin commented May 20, 2024

lpil commented May 24, 2024

omprakaash commented Apr 18, 2024 •

edited

omprakaash Apr 22, 2024 •

edited

omprakaash Apr 22, 2024 •

edited