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

sem: improve and rework semobjconstr #811

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Jul 22, 2023

Summary

Rework the semantic analysis of nkObjConstr nodes. The logic is now
simpler, compile-time evaluation happens from left to right, non-follow-
up errors are not dropped, and less follow-up errors are reported.

Details

The idea is to split the processing into two phases:

  1. AST validation and semantic analysis (i.e. figuring out the meaning)
  2. filling in the gaps (e.g., fitting types) and making sure language
    rules are not violated

Phase 1

During the first phase, for each slot, it is verified that the AST has
the expected shape and that a field with the given name exists in the
used object type; the initializer expression is also semantically
analyzed.

If the AST for a field assignment is not as expected, it is not analyzed
further. Compared to the previous logic, the "invalid field assignment"
error is not specialized further -- this is left to error message
rendering (currently astDiagToLegacyReport), thus making
adSemFieldAssignmentInvalidNeedSpace obsolete.

Errors in phase 1 do not short-circuit the second phase.

Phase 2

The second phase is split up into two parts:

  1. post-processing of the initializer expression and basic validity
    checks that only need the object construction expression
  2. validity checks that also need to consider the object type

The post-processing includes (if possible) folding the initializer for
discriminators and fitting the expression's type to the formal type; the
basic validity checks are the field visibility and duplicated field
checks. Errors nodes produces here are placed directly into the
temporary AST.

This allows for dropping the semExprFlagDispatched procedure and the
efPreferStatic, efPreferNilResult, and efPreferStatic expression
flags.

Running the checks that require traversal/inspection of the object type
re-uses the pre-existing semConstructTypeAux and semConstructFields
(both which have their sem prefix replaced by check). Both only
inspect the construction expression now (no sem-checking or modification
is performed) and rely on the field assignments that reach them to either
be an nkError node or valid, which significantly simplifies error
handling.

Since it is obsolete now, the rsemFieldOkButAssignedValueInvalid is
removed. A specialized wrapper for incorrect object constructions is not
needed (the generic wrapper suffices), so
adSemObjectConstructorInvalid is also removed.

Summary
=======

Rework the semantic analysis of `nkObjConstr` nodes. The logic is now
simpler, compile-time evaluation happens from left to right, non-follow-
up errors are not dropped, and less follow-up errors are reported.

Details
=======

The idea is to split the processing into two phases:
1. AST validation and semantic analysis (i.e. figuring out the meaning)
2. filling in the gaps (e.g., fitting types) and making sure language
   rules are not violated
@zerbina zerbina added compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Jul 22, 2023
The duplicate field name was not set, thus crashing the compiler. In
addition, explicitly document the error precedence.
The check tested for whether `discriminator` is an error node (it never
is) instead of `discriminatorVal`, causing an
`rsemRuntimeDiscriminantInitCap` to be reported.
@zerbina zerbina requested a review from saem August 5, 2023 20:58
@zerbina zerbina marked this pull request as ready for review August 5, 2023 20:58
@saem
Copy link
Collaborator

saem commented Aug 5, 2023

Just an initial observation, this separates expansion/synthesis (phase 1) and reduction/inheritance (phase 2). Definitely reads a lot cleaner and the amount of lines removed is great.

I'm doing a trace of the logic just to make sure I follow along, but a big fan thus far. 🥳

Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Much improved. I followed along the logic and it seems to cover all the bases, nicely done.

@saem
Copy link
Collaborator

saem commented Aug 6, 2023

/merge

@github-actions
Copy link

github-actions bot commented Aug 6, 2023

Merge requested by: @saem

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


To-do

  • add tests for the changes
  • write a proper commit message

Notes for Reviewers

  • a small side-project; the complexity in semobjconstr has been bothering me for a while
  • I wanted to use ElaborateAst here, but it doesn't work well for semObjConstr

@chore-runner chore-runner bot added this pull request to the merge queue Aug 6, 2023
Merged via the queue into nim-works:devel with commit 3b21895 Aug 6, 2023
27 checks passed
@zerbina zerbina deleted the partial-rework-of-semobjconstr branch August 6, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants