WIP: Derive Inspect #5775

rtfeldman · 2023-08-29T01:46:37Z

No description provided.

github-actions · 2023-10-13T02:07:44Z

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!

crates/compiler/can/src/derive.rs

ayazhafiz · 2023-11-28T23:36:01Z

crates/compiler/builtins/roc/Dict.roc

@@ -126,6 +130,11 @@ isEq = \xs, ys ->
 hashDict : hasher, Dict k v -> hasher where k implements Hash & Eq, v implements Hash, hasher implements Hasher
 hashDict = \hasher, dict -> Hash.hashUnordered hasher (toList dict) List.walk

+toInspectorDict : Dict k v -> Inspector f where k implements Inspect & Hash & Eq, v implements Inspect, f implements InspectFormatter


Suggested change

toInspectorDict : Dict k v -> Inspector f where k implements Inspect & Hash & Eq, v implements Inspect, f implements InspectFormatter

toInspectorDict : Dict k v -> Inspector f where k implements Inspect, v implements Inspect, f implements InspectFormatter

Those are redundant, right?

It is required for some reason:

This expression is used in an unexpected way: 133│ toInspectorDict : Dict k v -> Inspector f where k implements Inspect, v implements Inspect, f implements InspectFormatter ^ This argument to an opaque type has type: k where k implements Inspect But you are trying to use it as: #a where #a implements Hash & Eq Note: The type variable k says it can take on any value that implements only the ability Inspect. But, I see that it's also used as if it implements the abilities Hash and Eq. Can you use k without those abilities? If not, consider adding them to the implements clause of k.

ayazhafiz · 2023-11-28T23:36:55Z

crates/compiler/builtins/roc/Inspect.roc

+# The current default formatter for inspect.
+# This just returns a simple string for debugging.
+# More powerful formatters will likely be wanted in the future.
+DbgFormatter := { data : Str }


Not important right now, but I wonder if we should split this out into a separate module.

Yeah, I debated the same, but ultimately thought it would be nicer to just have Inspect.toDbgStr. I am open to either, but that can be dealt with in follow up.

crates/compiler/can/src/derive.rs

crates/compiler/derive/src/inspect.rs

crates/compiler/solve/src/specialize.rs

ayazhafiz · 2023-11-28T23:49:24Z

crates/compiler/test_mono/src/tests.rs

@@ -3289,3 +3289,150 @@ fn non_nullable_unwrapped_instead_of_nullable_wrapped() {
        "#
    )
 }
+
+#[mono_test]
+#[ignore = "Hits an unimplemented for abilities, not sure why..."]


Is this still the case?

Yes, still fails, also fails in a similar test for encode. Haven't dug into it much:

thread 'inspect_custom_type' panicked at '[Abilities] not yet implemented. Tracking issue: https://github.com/roc-lang/roc/issues/2463. Additional information: Not reachable yet', crates/compiler/mono/src/layout.rs:511:55

ayazhafiz · 2023-11-28T23:51:01Z

crates/compiler/types/src/subs.rs

@@ -1754,6 +1756,7 @@ impl Subs {
        symbol_names.push(Symbol::HASH_HASHER);
        symbol_names.push(Symbol::HASH_HASH_ABILITY);
        symbol_names.push(Symbol::BOOL_EQ);
+        symbol_names.push(Symbol::INSPECT_INSPECT_ABILITY);
        // END INIT-SymbolNames


Curse of IFTTT... we should rename to "INIT-SymbolSubsSlice" ( sorry, I know it's not due to this PR)

I don't truly follow, but renamed.

crates/compiler/uitest/tests/ability/specialize/inspect/opaque_derived.txt

crates/compiler/uitest/tests/ability/specialize/inspect/opaque_custom_impl.txt

ayazhafiz · 2023-11-28T23:52:31Z

I approved, but if possible, we should address the comments related to things introduced in this PR (the other stuff, which was introduced previously, I can fixup)

`specialize/inspect/opaque_derived` does not yet pass.

bhansconnect · 2023-11-29T04:11:41Z

@rtfeldman, can we force merge this. Seems that the CI runner is just broken currently. This PR and a few other are just waiting in queue. I have run all the tests locally which is a nix macos apple silicon machine. Also, nothing was changed for any dev tools.

rtfeldman force-pushed the inspect-derive branch from 263fb56 to 720eaaf Compare September 2, 2023 01:29

rtfeldman force-pushed the inspect-derive branch from f88ea97 to c7fb2ac Compare September 12, 2023 16:36

github-actions bot added inactive for 30 days and removed inactive for 30 days labels Oct 13, 2023

lukewilliamboswell mentioned this pull request Nov 3, 2023

Compiler expectation broken while playing with tutorial #5949

Open

bhansconnect force-pushed the inspect-derive branch 3 times, most recently from c1dd9f9 to 481dcea Compare November 28, 2023 20:56

bhansconnect reviewed Nov 28, 2023

View reviewed changes

crates/compiler/can/src/derive.rs Show resolved Hide resolved

bhansconnect marked this pull request as ready for review November 28, 2023 21:10

bhansconnect force-pushed the inspect-derive branch from 710b395 to 30f9515 Compare November 28, 2023 21:24

ayazhafiz self-requested a review November 28, 2023 21:49

bhansconnect force-pushed the inspect-derive branch from 7474b25 to 3eb6f91 Compare November 28, 2023 22:02

ayazhafiz previously approved these changes Nov 28, 2023

View reviewed changes

bhansconnect dismissed ayazhafiz’s stale review via e89265b November 29, 2023 00:29

rtfeldman and others added 13 commits November 28, 2023 16:40

Add Nat to Inspect ability

5a872a1

Add Inspect to obligation checker

fb9e0fc

Add derive key for Inspect

d0841fb

Add Inspect to builtin_module_with_unlisted_ability_impl

00c27b0

wip

3414864

TODO

c87e3e7

Get first inspect for non-Inspect-implementing opaques specialized

5c805ce

Add a few more opaque tests

d36eba9

`specialize/inspect/opaque_derived` does not yet pass.

Add Inspect.Inspect to subs and auto-derive

443f659

Add some Inspect.inspect tests, fix a bug

768b55b

add DbgFormatter to inspect

b8e644a

encode/encoder to inspect/inspector

96b3cc3

add uitest to view bool inspect derive

c57bc4d

bhansconnect added 23 commits November 28, 2023 16:40

allow nat in DeriveInspect

35078a2

correct Inspect.opaque and Inspect.function to be function calls

f2ab6b5

unwrap builtin opaques like NUM_NUM

796c2b1

add Debug to FlatInspectable

6eedd08

use INSPECT_INSPECT_ABILITY instead of INSPECT_INSPECT

82cda19

check_adhoc for inspect

c443bdc

expand ui tests for inspect

4c25c60

correct Inspect custom impl to use Inspect.apply

5e36395

add a set of currently failing mono tests for inspect

21b68a2

add walk function to list autoderive

153b4c6

ignore unimlemented ability test case

32c3d49

fix mono tests with wrapping custom function

79a5884

add inspect implementation for dict and set

9b181e1

update inspect examples

aeabf96

update tests

83bf3cb

roc format

759fa9b

update cli logging test

77e55c8

more formatting

0586f2b

update gen_abilities tests to include edge case

4b91152

remove empty list test case for inspect

01032c3

deal with Frac * -> Dec

1b7fdc9

misc cleanup suggestions

c490462

fix wasm tests

248976d

bhansconnect force-pushed the inspect-derive branch from e89265b to 248976d Compare November 29, 2023 00:41

bhansconnect enabled auto-merge November 29, 2023 16:18

Anton-4 approved these changes Nov 29, 2023

View reviewed changes

bhansconnect merged commit ead9031 into main Nov 29, 2023
16 checks passed

bhansconnect deleted the inspect-derive branch November 29, 2023 16:22

lukewilliamboswell mentioned this pull request Apr 26, 2024

Update the Abilities guide for Inspect #6679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Derive Inspect #5775

WIP: Derive Inspect #5775

rtfeldman commented Aug 29, 2023

github-actions bot commented Oct 13, 2023

ayazhafiz Nov 28, 2023

bhansconnect Nov 29, 2023

ayazhafiz Nov 28, 2023

bhansconnect Nov 29, 2023

ayazhafiz Nov 28, 2023

bhansconnect Nov 29, 2023

ayazhafiz Nov 28, 2023

bhansconnect Nov 29, 2023

ayazhafiz commented Nov 28, 2023

bhansconnect commented Nov 29, 2023

	toInspectorDict : Dict k v -> Inspector f where k implements Inspect & Hash & Eq, v implements Inspect, f implements InspectFormatter
	toInspectorDict : Dict k v -> Inspector f where k implements Inspect, v implements Inspect, f implements InspectFormatter

WIP: Derive Inspect #5775

WIP: Derive Inspect #5775

Conversation

rtfeldman commented Aug 29, 2023

github-actions bot commented Oct 13, 2023

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

ayazhafiz commented Nov 28, 2023

bhansconnect commented Nov 29, 2023