Use new printer for signature help #4589
Conversation
scristobal
force-pushed
the
issue-4570-minimal
branch
from
ca81a44 to
1ef7e2e
Compare
|
Hello! Is this ready for a review? |
|
Hi @scristobal, are you still working on this? |
scristobal
force-pushed
the
issue-4570-minimal
branch
4 times, most recently
from
08e9b98 to
d51a6c4
Compare
Ups, sorry busy weeks at work lately. I think this should be there. Thanks for your patience! |
|
Not at all, thank you! |
compiler-core/src/type_/printer.rs
Outdated
| _ = self.type_variables.insert(id, local_alias.clone()); | ||
| } | ||
|
|
||
| pub fn map_new_variable(&mut self, old_id: u64, new_id: u64) { |
There was a problem hiding this comment.
Could you document what this does please? 🙏
I was a bit confused by the word "map" as in my mind that means a function like list.map. Is there a name that someone like me with less context would find extra clear?
There was a problem hiding this comment.
With pleasure!
renamed the function to reassign_type_variable_alias which is much more explicit, also added a small comment on top describing what it does. Hope it helps!
| let v = self.new_unbound_var(); | ||
| let _ = ids.insert(*id, v.clone()); | ||
| let new_id = self.previous_uid(); | ||
| self.names.map_new_variable(*id, new_id); |
There was a problem hiding this comment.
This looks good. What was the situation in which it wouldn't work without this change? A comment explaining it would be helpful for future readers.
There was a problem hiding this comment.
Absolutely, great feedback!
I added a comment explaining why this is necessary, also changed new_id name which was confusing, coming from a previous id... 🤔
I hope it is more clear now!
scristobal
force-pushed
the
issue-4570-minimal
branch
from
d51a6c4 to
04bce9b
Compare
|
|
||
| ----- Signature help ----- | ||
| guard(Bool, a, fn() -> a) -> Float | ||
| guard(Bool, b, fn() -> b) -> Float |
There was a problem hiding this comment.
Do you know why this has changed? The generics are named a, so this should print a, not b here.
scristobal
force-pushed
the
issue-4570-minimal
branch
from
921ed4c to
c957690
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| // Preserve any user-provided name from the original generic variable | ||
| // for the new unbound variable. This ensures error messages and type | ||
| // displays continue to use meaningful names (e.g., "something") rather | ||
| // than auto-generated ones (e.g., "a", "b"). |
There was a problem hiding this comment.
Doesn't this mean that the previous variable no longer has the name? Instead of both of them having that name due to their pointing to the same type.
Maybe this is the cause of the behaviour @GearsDatapacks pointed out above.
|
Do you know why there's some AI review thing in this PR? It doesn't look like it's enabled for this repo so I don't understand why it's here. |
From the CHANGELOG:
The function signature helper now displays original function definition
generic names when arguments are unbound. For example, in this code:
will show a signature help
instead of
resolves #4570