Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std/json: remove macro type API usage #825

Merged

Conversation

saem
Copy link
Collaborator

@saem saem commented Aug 5, 2023

Summary

Remove usage of macros type API usage in std/json, specifically
getTypeImpl and getTypeInst. This is part of a broader effort to
drop this API from the standard library. The current API enforces a
number of poor design decisions in compiler internals. The API itself
is questionable and much of this work should happen via generics.

Details

As part of removing the usage of these APIs, there was a slight
regression in the precision of errors when a tuple without named fields
is used. The error message is equivalent, but the error location will
be within the json module itself.

The change itself uses the fieldPairs iterator which relies upon
compiler support in semfields. Some minor adjustments were made here
in particular the ability to handle nkError nodes.

The json module relies upon the ability to forward declare generic
procedures, this was not possible with the earlier bootstrap compilers,
and existed behind a conditional compilation flag
(nimFixedForwardGeneric). Seeing as this is no longer the case the
flag has been removed as part of the module rework.

Finally, the test tjsonmacro_reject existed for the previous approach,
but the test are combined into tjsonmacro and the previous test is
removed.

`containsNode` previously attempted to traverse `nkError` nodes,
resulting in run time error as there is no child nodes field (`sons`).
little ugly, but this is the first pass proving it out for `object` and
`tuple` types.
@saem saem added stdlib Standard library simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Aug 5, 2023
@saem
Copy link
Collaborator Author

saem commented Aug 11, 2023

Next thing to fix is handling the error pragma handling, presently only in the VM, it probably needs to be in sempass2, as well.

error in macros is implemented a bit oddly, it can only execute in the VM and it's far too general a name. the whole api is infuriating, what should be a simple static call that can be folded is instead tangled up in macro land.

lib/pure/json.nim Outdated Show resolved Hide resolved
@saem saem marked this pull request as draft August 14, 2023 19:01
@saem saem changed the title std json remove macro type api usage std/json: remove macro type API usage Aug 15, 2023
@saem saem marked this pull request as ready for review August 15, 2023 18:47
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! It's a lot less code now, and the version using generics is much easier to understand too. I've left a few clean-up suggestions.

As an aside, the removal of nimFixedForwardGeneric makes the final change-set a bit hard to read. Turning the removal into a separate PR would be nice, but it's not something blocking -- no change expected from my side here.

@@ -986,7 +984,7 @@ proc skipGenericInvocation(t: PType): PType {.inline.} =

proc addInheritedFields(c: PContext, check: var IntSet, pos: var int,
obj: PType) =
assert obj.kind == tyObject
assert obj.kind == tyObject, $obj.kind
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert obj.kind == tyObject, $obj.kind
assert obj.kind == tyObject

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be easier to debug by leaving the kind in there?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would, although I personally think that assertions with $x are visually a bit noisy, hence my suggestion. It's nothing critical, however, and I'm fine with it staying as it is.

It's nothing for this PR, but since asserting that the discriminator of some object has a certain value is a common pattern in the compiler, I think that adding a new idiom assertKind (which automatically renders the value on failure) would make sense.

lib/pure/json.nim Outdated Show resolved Hide resolved
lib/pure/json.nim Outdated Show resolved Hide resolved
lib/pure/json.nim Outdated Show resolved Hide resolved
lib/pure/json.nim Outdated Show resolved Hide resolved
@zerbina
Copy link
Collaborator

zerbina commented Aug 15, 2023

I've applied some small typo and grammar fixes to the PR message.

@saem saem marked this pull request as draft August 16, 2023 18:32
saem and others added 4 commits August 16, 2023 11:32
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
@saem saem marked this pull request as ready for review August 16, 2023 18:39
compiler/ast/ast_query.nim Outdated Show resolved Hide resolved
saem and others added 2 commits August 17, 2023 15:37
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
@saem
Copy link
Collaborator Author

saem commented Aug 18, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • part of a bigger sweep through the stdlib to remove all such API usage

@chore-runner chore-runner bot added this pull request to the merge queue Aug 18, 2023
Merged via the queue into nim-works:devel with commit 2eed234 Aug 18, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
simplification Removal of the old, unused, unnecessary or un/under-specified language features. stdlib Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants