Add record constructor to subtyping

[Eckaos]

[Eckaos]

@yannham What I did here is pretty much unify for record rows. It miss TailVar, Constant, etc... Do you know how to reconstruct a GenericUnifType from GenericUnifRecordRows. For the time being, I reconstruct it from the data I have. I may have reconstruct the type with the wrong levels.

[yannham]

Do you know how to reconstruct a GenericUnifType from GenericUnifRecordRows.

The best, if those record rows are coming from destructuring a unif type to begin with, is to keep the original var_levels around so that you can reconstruct the original type.

If this type is modified, or generated, or coming from some type inference transformation, you can just use UnifType::concrete (or GenericUnifType::concrete), the method with a lowercase c (and not the constructor Concrete), which takes care of updating and filling the levels accordingly.

[Eckaos]

The commit 58af17d break stuff about record. Left to do :

• [x] Comments
• [x] Fix the problem about records
• [x] Fix warning

[Eckaos]

The problem about records wasn't real, it was just a simple problem about reporting error correctly. Another problem arose. It is about lsp/nls/tests/inputs/hover_field_typed_block_regression_1574.ncl, it give a new snapshot and I don't know why. @yannham

If my fix does work, the work left to do is to :

• [x] Add documentation
• [x] Update comments
• [x] Add tests
• [x] Remove subsumption function in typecheck/mod.rs

[Eckaos]

To de-duplicate the code between is_subsumed_by and unify, we can just call unify where we need in is_subsumed_by. The cost, I see, will be the cost of pattern matching + function call (depends on what optimization the rust compiler will do). Or we could abstract cases that are the same in is_subsumed_by and unify into another function. The cost, I see here, will be pattern matching + function call in the two functions (depends on what optimization the rust compiler will do). @yannham

[yannham]

To de-duplicate the code between is_subsumed_by and unify, we can just call unify where we need in is_subsumed_by.

I'm not sure to understand what you mean here. For example, when subsuming record types, we do the same general work but we still recursively calls is_subsumed_by on each component, so we can't just replace the whole case by a call to unify. Indeed, I think we need to have a core function that is generic how it calls itself recursively. It's certainly doable, but we can do that in a second step.

P.S: it could be good to have tests for more complicated record types: types with tails, record types with subsumption acting on several components, etc.

[Eckaos]

I was talking about cases where we do exactly the same work (with Constant and UnifVar etc...). In those cases, we can call unify from is_subsumed_by. The other alternative was to factor out a function which does only cases where it is the same result for unify and is_subsumed_by and then call it from unify and is_subsumed_by when needed.

[github-actions[bot]]

Bencher

Report	Fri, September 6, 2024 at 15:27:59 UTC
Project	nickel
Branch	2007/merge
Testbed	ubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

• Latency (latency)

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns)
fibonacci 10	➖ (view plot)	504,530.00
pidigits 100	➖ (view plot)	3,192,200.00
product 30	➖ (view plot)	796,650.00
scalar 10	➖ (view plot)	1,496,400.00
sum 30	➖ (view plot)	793,290.00

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

[Eckaos]

I don't understand why cargo and the continuous integration pipeline give me an error about the lsp. @yannham

[jneem]

It's because the lsp snapshot tests have changed -- you've changed docs in the stdlib and so the hover text in the lsp is different. Running cargo test -p nickel-lang-lsp and cargo insta review locally should let you update the snapshots.

[yannham]

It's because the lsp snapshot tests have changed -- you've changed docs in the stdlib and so the hover text in the lsp is different. Running cargo test -p nickel-lang-lsp and cargo insta review locally should let you update the snapshots.

I don't see any change to the stdlib. Also, the diff of the snapshot is dubious - we now get an additional monomorphic type with _a coming first in the LSP output. So it seems this PR has changed something related to either how TypecheckVisitor is called, or to the apparent type of values, etc.

[yannham]

I'm taking over this PR for the foreseeable future, unless @Eckaos thinks he has the time to bring it down the finish line.

[yannham]

After some thorough investigation, I think I understand what's happening and I convinced myself that it makes sense (although in the test case it adds a bit of noise, it's also more correct in some cases - see the commit message). It an artifact of when we visit the term compared to when the types are instantiated, more or less.