Rank arrays #74

pbeart · 2025-03-19T13:45:15Z

No description provided.

…ore sense

VonTum

So, the vast majority (90%+) of line changes were due to renames of commonly used names. Don't do this, it severely slows down the review process.

Your work has some promising parts, namely the new Peano work, but also many strange and wildly incorrect parts, like adding peano types to Linker.

It's a promising direction, but to start we'll need to revert all those renames, as well as the clippy warnings that these changes cause.

I think it might be easiest to copy the Peano work (from abstract_type.rs), and create a clean branch to start over.

VonTum · 2025-03-27T23:53:04Z

src/prelude.rs

-    const DISPLAY_NAME: &'static str = "type_";
+pub struct WholeTypeUUIDMarker;
+impl UUIDMarker for WholeTypeUUIDMarker {
+    const DISPLAY_NAME: &'static str = "whole_type_";


Don't rename very commonly used types, like TypeUUID. That only causes headaches and merge conflicts with other work.

The change to rank arrays has nothing to do with the IDs we assign to types in the compilation context. "WholeType" is a good name for the combination of a named (IE, indexed by TypeUUID) and Peano array rank type.

Revert this rename.

VonTum · 2025-03-27T23:57:32Z

src/linker/mod.rs

@@ -214,7 +214,9 @@ enum NamespaceElement {
 ///
 /// Incremental operations such as adding and removing files can be performed on this
 pub struct Linker {
-    pub types: ArenaAllocator<StructType, TypeUUIDMarker>,
+    pub whole_types: ArenaAllocator<StructType, WholeTypeUUIDMarker>,


Please don't rename commonly used fields in Linker. The types field refers to all declared structs in the compilation context. A "whole type" is constructed locally, from a named type (referring to an element in Linker::types) and a Peano number (that is constructed locally)

Revert this rename

VonTum · 2025-03-28T00:00:00Z

src/linker/mod.rs

-    pub types: ArenaAllocator<StructType, TypeUUIDMarker>,
+    pub whole_types: ArenaAllocator<StructType, WholeTypeUUIDMarker>,
+    pub inner_types: ArenaAllocator<StructType, WholeTypeUUIDMarker>,
+    pub rank_types: ArenaAllocator<StructType, WholeTypeUUIDMarker>,


The "array ranks" are not something that is collected over multiple source files. the "Rank" arithmetic we've been talking about is a convenient way of representing numbers.

The Linker global singleton struct is used to collect all modules, types and constants that are declared in the user's source files. Since numbers don't need to have some central "store" of these, we don't need these here.

So, inner_types and rank_types should be removed

VonTum · 2025-03-28T00:03:11Z

src/flattening/mod.rs

@@ -804,6 +806,7 @@ impl Instruction {
 /// See [crate::typing::type_inference::HindleyMilner]
 #[derive(Debug, Clone)]
 pub struct TypingAllocator {
-    pub type_variable_alloc: UUIDAllocator<TypeVariableIDMarker>,
+    pub peano_variable_alloc: UUIDAllocator<PeanoVariableIDMarker>,
+    pub type_variable_alloc: UUIDAllocator<InnerTypeVariableIDMarker>,


So in another comment I complained about renaming TypeUUID, but in this case the splitting of TypeVariableIDMarker into PeanoVariableIDMarker and InnerTypeVariableIDMarker is justified. Keep 😄

Perhaps even take it further, by renaming type_variable_alloc to ´inner_type_variable_alloc´

So indeed to contrast it with my complaints about adding the rank stuff to Linker. Having this new peano allocator in TypingAllocator is correct because we indeed have to construct and typecheck the arrays locally within the module

VonTum · 2025-03-28T00:05:26Z

src/flattening/typechecking.rs

            whole_span,
-            "variable reference",
+            &"variable reference",


Do you have clippy enabled? It should warn on this unneccessary &.

To enable, add "rust-analyzer.check.command": "clippy", to your .vscode/settings.json

Hmm, you've added the reference it in multiple places? Why?

VonTum · 2025-03-28T00:20:41Z

src/typing/abstract_type.rs

    pub domain_substitutor: TypeSubstitutor<DomainType, DomainVariableIDMarker>,
+    //pub type_map: HashMap<Type


Clumsy comment?

VonTum · 2025-03-28T00:23:08Z

src/typing/abstract_type.rs

    ) {
+        // todo: tidy these up (remove duplication of code)


yeah some kind of abstraction combining unify inner and unify peano would be helpful

VonTum · 2025-03-28T00:25:37Z

src/typing/abstract_type.rs

        span: Span,
-        context: Context,
+        context: &Context,


Hmm, we need a better system for these Contexts, otherwise we're doing redundant clones all over the place

VonTum · 2025-03-28T00:26:13Z

src/typing/abstract_type.rs

@@ -413,7 +560,7 @@ impl TypeUnifier {
        span: Span,
        context: Context,
    ) {
-        self.typecheck_write_to_abstract(&found.typ, &expected.typ, span, context.clone());
+        self.typecheck_write_to_abstract(&found.typ, &expected.typ, span, &mut context.clone());


Redundant &mut. Also &mut of .clone()? That's a heavy code smell

VonTum · 2025-03-28T00:28:17Z

src/typing/type_inference.rs

@@ -253,7 +259,7 @@ impl<MyType: HindleyMilner<VariableIDMarker> + Clone + Debug, VariableIDMarker:
        found: &MyType,
        expected: &MyType,
        span: Span,
-        reporter: Report,
+        reporter: &Report,


Pass-by-value was on purpose to reduce clones. But we should find a better way of doing it. So we can get 0, 1, or 2+ copies without cost. Maybe the most important thing would be the 0 copies case, as we expect no type errors for the happy path

VonTum · 2025-03-28T03:50:34Z

I just committed my .vscode files for all of us to use. This includes rustfmt rules, as well as enabling clippy

4f2cdf7

pbeart added 9 commits March 6, 2025 13:03

Initial work on Peano types + unifier

a743acc

Work adding rank types

0c5505f

More work adding rank types (piecemeal!)

5d3b1f4

Rewrite inner and peano UUIDs back to WholeTypeUUIDs since it makes m…

0e14d9c

…ore sense

Cleanup

789532b

Cleanup

379f646

Cleanup again (squishme)

c78ce6a

Array type unifying + unify peanos when unifying built in operators

22a0a7f

Cleanup

7d1390b

VonTum mentioned this pull request Mar 24, 2025

Ranged Integers #66

Open

pbeart added 3 commits March 25, 2025 19:55

Return to single type lookups

c9ebb53

Finally, properly working ranked types (hacky hacks not withstanding)

201dfa2

Cleanup + remove leftover arrays in inner types

dcd059f

VonTum self-requested a review March 27, 2025 23:48

VonTum requested changes Mar 28, 2025

View reviewed changes

pbeart added 2 commits March 29, 2025 22:39

Initial bad rank-polymorphic concrete type implementation

6dafb9e

Incorporate array slicing

885909c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rank arrays #74

Rank arrays #74

pbeart commented Mar 19, 2025

VonTum left a comment

VonTum Mar 27, 2025

VonTum Mar 27, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum Mar 28, 2025

VonTum commented Mar 28, 2025 •

edited

Loading

		pub domain_substitutor: TypeSubstitutor<DomainType, DomainVariableIDMarker>,
		//pub type_map: HashMap<Type

Rank arrays #74

Are you sure you want to change the base?

Rank arrays #74

Conversation

pbeart commented Mar 19, 2025

VonTum 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VonTum commented Mar 28, 2025 • edited Loading

VonTum commented Mar 28, 2025 •

edited

Loading