fix: proper type checking on targets #3
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Blocktypes can define their own custom yants types and they will be checked properly, giving us some actual type safety.
nrdxp
force-pushed
the
type-check-blocks
branch
from
83d7777
to
10c9362
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Q: should we use uncheckd imported for the registry (i.e. the
extractor)?
- would that have any noticeable impact on registry eval?
- if it's a drv it's already not lazy as soon as .meta is queried
- when the type is `drv`, the check in yants is
`drv = typedef "derivation" (x: isAttrs x && x ? "type" && x.type == "derivation");`
if that's not lazy, then we incur in extra costs for all items that
otherwise wouldn't have needed to eval a drv
- practically speaking, the time real sensitive eval is `init`
- but `init` already incurs in an evaluation for `meta` (display)
- type checking _could_ have a negative impact when lib.lazyDerivation
is used, as this only shields `.meta` & `.passthru`, but not `.type`?
- A: we're lucky, `.type` is lazy in lazyDerivation, see:
https://github.com/NixOS/nixpkgs/blob/29f41f2a5d31f04e58334376dc08ced7212df197/lib/derivations.nix#L90
- `.type` or `isAttrs` breaking laziness needs to be validated
blaggacao
reviewed
Feb 12, 2023
| # extract instatiates actions and extracts metadata for the __std registry | ||
| extracted = l.optionalAttrs (cellBlock.cli or true) (l.mapAttrs (_extract cellBlock) imported); | ||
| extracted = l.optionalAttrs (cellBlock.cli or true) (l.mapAttrs (_extract cellBlock) imported'); |
There was a problem hiding this comment.
The big question: type-checked imported' here (for the extractor → registry) or unchecked imported?
I left a couple of considerations in the commit message...
blaggacao
approved these changes
Feb 12, 2023
blaggacao
force-pushed
the
main
branch
3 times, most recently
from
664bd54
to
b83335b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Blocktypes can define their own custom yants types and they will be checked properly, giving us some actual type safety.
Leaving in draft until I can implement some times in Standard upstream and test this properly. Also need to double check if this is breaks laziness.