fix: style relation checker using substance variables #1239
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
Deploying with
|
| Latest commit: |
3dcb5cf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e6a96a2e.penrose-72l.pages.dev |
| Branch Preview URL: | https://merging-envs.penrose-72l.pages.dev |
± Registry diff📊 PerformanceKeyNote that each bar component rounds up to the nearest 100ms, so each full bar is an overestimate by up to 400ms. Data |
Codecov Report
@@ Coverage Diff @@
## main #1239 +/- ##
==========================================
+ Coverage 63.13% 63.45% +0.32%
==========================================
Files 60 61 +1
Lines 7061 7170 +109
Branches 1646 1664 +18
==========================================
+ Hits 4458 4550 +92
- Misses 2519 2537 +18
+ Partials 84 83 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
liangyiliang
requested review from
samestep and
wodeni
and removed request for
wodeni
|
Just to clarify, does #1123 become fixed via this PR? If so, could you modify your PR description autoclose it using GitHub's special syntax? https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue |
Done! |
wodeni
approved these changes
Jan 18, 2023
samestep
approved these changes
Jan 18, 2023
3 tasks
samestep
reviewed
Jan 18, 2023
Co-authored-by: Sam Estep <sam@samestep.com>
Co-authored-by: Sam Estep <sam@samestep.com>
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.
Description
Related issue/PR: #1123
This PR fixes the uncaught error in the Style compiler when selector relations refer to a variable not declared in the selector, but exists in the substance program. It now throws an error. We added a test case on this error. Finally, we improved upon how the "suggested" variables in the error message distinguish between Style and Substance variables.
Implementation strategy and design decisions
The style compiler checks through the relations in the
whereclause, primarily using functions within the Substance checker, which checks the consistency of expressions with respect to aSubstanceEnv. In particular, it checks that each variable referred in the expressions are declared insideSubstanceEnv.In order to call the Substance checker, we would need to provide a
SubstanceEnvthat contains all the variables that we want the relations to be able to access. Previously, we just take the currentSubstanceEnv(as used in the Substance program), and add all the variables declared in the style header. The problem is that now thisSubstanceEnvcontains both variables declared in the style header (which we want to be able to access) and variables not declared in the style header but exists in the substance program.The solution is simply to create a new, empty
SubstanceEnvand add in the variables declared in the style header.Examples with steps to reproduce them
This is the same example as used in the original issue, running in the version in this PR. Previously, this did not trigger an error, and only fails silently under the browser console. Now it does trigger a sensible error.
Checklist