Automatic annotation of type signatures #7130

snobee · 2024-09-29T04:09:12Z

Resolves #7042

I've started with all the logic in analysed_doc but I'll probably end up moving some of it into fmt when I start on that part

I'm making some assumptions here that I wanted to confirm:

All definitions begin at the start of a line
The left side of a type annotation is the exact same as the left side of a definition

snobee · 2024-11-06T03:24:36Z

crates/cli/src/format.rs

+    buffer
+}
+
+pub fn annotation_edits(


I've exposed this and annotations_edit() publicly to be used in the language server but It feels weird to expose them from the cli crate. If it should be in another crate what one?

crates/cli/src/format.rs

snobee · 2024-11-06T03:24:57Z

crates/cli/src/main.rs

+
+            let annotate_exit_code = match annotate_file(&arena, roc_file_path.to_owned()) {
+                Ok(()) => 0,
+                Err(LoadingProblem::FormattedReport(report)) => {


If there's a problem loading the file then it just prints out the error. I know this needs to be updated but I'm not sure what the error message should be. Should it include the loading problem or just say that there was one?

snobee · 2024-11-06T03:26:27Z

I've added some comments on the code with some questions about things that I'm not sure about

There's a test that's failing in the language server and that's on purpose. I'm pretty sure the problem is in #7126 but don't know how to check that until it's been fixed, so I'm going to consider this blocked. I'll take a look but I'm not very confident I can figure it out.

github-actions · 2024-12-07T02:07:59Z

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

snobee · 2024-12-14T00:32:02Z

This has been open long enough so I've put it up for review. There's still issues with suffixed expressions from #7126 but the code for this is independent from that.

snobee · 2024-12-14T04:51:26Z

I changed an if to an assert last minute 🙃. That should fix it

lukewilliamboswell · 2025-01-06T00:16:34Z

@snobee -- can you please merge latest main?

Apologies for leaving it so long... I think this slipped through and got forgotten about.

snobee · 2025-01-16T22:35:56Z

@lukewilliamboswell No problem 😃 Should be ready for review now!

joshuawarner32 · 2025-01-20T02:23:41Z

crates/language_server/src/analysis/annotation_visitor.rs

+            if self.should_visit(region) {
+                if let Expr::Call(_, args, CalledVia::QuestionSuffix) = expr {
+                    let Expr::Closure(ClosureData { arguments, .. }) = &args[1].1.value else {
+                        internal_error!("Suffixed expression did not contain a closure")


Particularly for the language server, I think we should be more careful about crashing. Is there a sensible default we can use here?

Just skipping this node and printing an error is probably the best way to handle it.

That being said, I can see that this form of question suffix desugaring was removed from the compiler, it's just the enum variant that remains. I think I can just remove this whole section instead.

@lukewilliamboswell do you know what the status is of CalledVia::QuestionSuffix?

joshuawarner32 · 2025-01-20T02:26:56Z

crates/language_server/src/analysis/analysed_doc.rs

+            decl.range,
+        );
+
+        let pos = roc_region::all::Position::new(offset as u32);


Can you add a use statement at the top for Position?

lsp_type::Position is already imported and I must have forgot about renaming imports. I'll import as RocPosition or something

Ahh I missed that there was another thing named Position. In that case, I don't feel strongly.

joshuawarner32 · 2025-01-20T02:27:20Z

crates/cli/src/format.rs

+            let symbol = decls.symbols[index];
+            let expr = &decls.expressions[index].value;
+
+            use roc_problem::can::RuntimeError::ExposedButNotDefined;


Should this use be moved to the top?

joshuawarner32 · 2025-01-20T03:05:15Z

crates/cli/src/format.rs

+    // Generated names for errors start with `#`
+    .replace('#', "");


It seems that if there's an error in the type, don't we want to cancel the operation, no?

maybe? These only seem show up when a type doesn't implement an ability it's supposed to. I'm not sure how I'd go about detecting all type errors. I could try looking for

Error

FlexVar(None) or FlexAbleVar(None, ...) that show up anywhere except as arguments

FlexVar or FlexAbleVar that have names that start with '#'

But that still allows things like List.first(8) which has the type Result a [ListWasEmpty]. I'm not sure if that would be counted as an error or not? a doesn't come from anywhere so maybe that means it is?

This stuff is a bit over my head so maybe I'm missing something but I don't see anything in the codebase for this already

The other way to disallow errors is not from the type but if the code itself has any warnings or errors but that seems a bit heavy handed

a doesn't come from anywhere so maybe that means it is?

I think (this is not really my area...) that may be fine, and the constraint is that a needs to be a fresh name (not bound in the outer scope).

Maybe @bhansconnect has more specific advice about how we should be applying types here?

I would short circuit and break out if there are any errors in types, I think it's too easy to generate wrong annotations otherwise. If the easiest method to implement it is to check if the code has any errors, I think that's fine at this moment in time

snobee force-pushed the annotate-type-signatures branch 4 times, most recently from f03b305 to 387c509 Compare November 5, 2024 23:48

snobee commented Nov 6, 2024

View reviewed changes

crates/cli/src/format.rs Outdated Show resolved Hide resolved

snobee commented Nov 6, 2024

View reviewed changes

github-actions bot added the inactive for 30 days label Dec 7, 2024

snobee force-pushed the annotate-type-signatures branch from 387c509 to acea61b Compare December 14, 2024 00:31

snobee marked this pull request as ready for review December 14, 2024 00:32

github-actions bot removed the inactive for 30 days label Dec 14, 2024

snobee added 5 commits December 13, 2024 20:46

Add format annotate subcommand

9f67c79

Add code action to annotate declarations

40a0086

Fix bug converting ranges to the wrong region

6e39425

Add tests for automatic annotation

6313461

Fix crash when no declarations are exposed or defined

d4e558d

snobee force-pushed the annotate-type-signatures branch from acea61b to d4e558d Compare December 14, 2024 04:49

snobee added 5 commits January 15, 2025 20:50

Use internal_error! instead of assert_debug!

78ba0ca

Remove duplicated Loc::byte_range()

1ec619f

Stop annotating recursion tags

c519ab7

Merge remote-tracking branch 'upstream' into annotate-type-signatures

406cc6c

Remove references to Task and CalledVia::BangSuffix

bbe2d7c

joshuawarner32 reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic annotation of type signatures #7130

Automatic annotation of type signatures #7130

snobee commented Sep 29, 2024

snobee Nov 6, 2024

snobee Nov 6, 2024

snobee commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Dec 7, 2024

snobee commented Dec 14, 2024

snobee commented Dec 14, 2024 •

edited

Loading

lukewilliamboswell commented Jan 6, 2025

snobee commented Jan 16, 2025

joshuawarner32 Jan 20, 2025

snobee Jan 21, 2025

joshuawarner32 Jan 21, 2025

joshuawarner32 Jan 20, 2025

snobee Jan 21, 2025

joshuawarner32 Jan 21, 2025

joshuawarner32 Jan 20, 2025

snobee Jan 21, 2025

joshuawarner32 Jan 20, 2025

snobee Jan 21, 2025 •

edited

Loading

joshuawarner32 Jan 23, 2025

ayazhafiz Jan 23, 2025

		// Generated names for errors start with `#`
		.replace('#', "");

Automatic annotation of type signatures #7130

Are you sure you want to change the base?

Automatic annotation of type signatures #7130

Conversation

snobee commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snobee commented Nov 6, 2024 • edited Loading

github-actions bot commented Dec 7, 2024

snobee commented Dec 14, 2024

snobee commented Dec 14, 2024 • edited Loading

lukewilliamboswell commented Jan 6, 2025

snobee commented Jan 16, 2025

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

snobee Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snobee commented Nov 6, 2024 •

edited

Loading

snobee commented Dec 14, 2024 •

edited

Loading

snobee Jan 21, 2025 •

edited

Loading