update contracts.dd to reflect DIP1009#2339
Conversation
|
Thanks for your pull request and interest in making D better, @zachthemystic! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
|
The DAutotest builder will show you a preview of how it looks. But it failed to build due to trailing whitespace. I fixed that for you. |
Much appreciated. |
spec/contracts.dd
Outdated
| $(P As of the acceptance of | ||
| $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md, DIP1009), | ||
| pre and post contracts can be written either: 1. in expression form, with | ||
| syntaxes similar to $(B assert), or 2. as block statements containing arbitrary |
There was a problem hiding this comment.
Remove the inline numbers here:
"... can be written either in expression form, with syntaxes similar to $(B assert), or as block statements containing arbitrary code."
The numbers are confusing, especially when each paragraph is numbered.
spec/contracts.dd
Outdated
| $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md, DIP1009), | ||
| pre and post contracts can be written either: 1. in expression form, with | ||
| syntaxes similar to $(B assert), or 2. as block statements containing arbitrary | ||
| code. The expression form is:) |
There was a problem hiding this comment.
Start a new paragraph for "The expression form is", so a new paragraph number shows up.
spec/contracts.dd
Outdated
| or at some point in the block statement. An `out` contract of the | ||
| expression form should omit the identifier when testing for anything other than | ||
| the return value. In the block statement form, the identifier (along with its | ||
| accompanying parentheses) should likewise be omitted if no test requires it. |
There was a problem hiding this comment.
This is really verbose. I think the syntax speaks for itself and doesn't need a full description.
Maybe "The optional identifier in either type of out contract is set to the return value of the function." That's pretty much it.
There was a problem hiding this comment.
I'll take your word for it. My objection would be that the documentation can also be intended to help beginners understand the intended use of the features. The whole document is written somewhat like that, as opposed to pure technical documentation. But as I said, I'll take your word for it.
There was a problem hiding this comment.
There are lots of places in the spec, where the syntax is not described in english. It's pretty clear to me in the syntax that you can omit the identifier if you don't intend to test it.
There was a problem hiding this comment.
Yeah, you're probably right. It's just that I needed to voice my objection in case there was any merit to it.
| statement or expression is allowed in the bodies, but it is important | ||
| parameters. If a post contract fails, then there is a bug in the function. In either case, | ||
| an `assert` statement within the corresponding `in` or `out` block should throw an | ||
| `AssertError`.) |
There was a problem hiding this comment.
should throw -> will throw
(the former makes it sound like it's something you have to ensure)
|
Okay, I added the grammar changes as well, but I'll need to wait for the DAutoTest to finish to check the html appearance. |
|
Uh, the DAutoTest failed for the commits adding the grammar files, but I don't know why. |
|
Wow, the build log is quite an explosion of errors. I'm not sure how to diagnose! I'm wondering if it's somehow actually testing the grammar to see if it's valid within dmd. |
|
ping @CyberShadow |
|
I got as far as realizing that the successful build also spews the errors. It spews about twice as much as the failed build. The failed build ended seemingly arbitrarily when it built LaTex, about halfway through the process. |
|
Uh oh, you have angered the PDF generator :) It seems to complain about this line somewhere: It's not part of this diff, so it's likely that some bit of syntax went through unescaped in the Latex intermediary file, which in turn caused some tag to not be properly closed (or prematurely closed). The next step would be to hunt which part of this diff causes the problem, and add escapes as necessary.
Nope, it doesn't - go to the DAutoTest page, click on the link by "Base result", then see the log linked there - there are lots of PDF warnings, but it ultimately succeeds. |
spec/contracts.dd
Outdated
| $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1003.md, DIP1003), | ||
| the keyword $(D body) was required instead of `do`, and may still be encountered in | ||
| old code bases. In the long term, $(D body) may be deprecated, but for now it's allowed both | ||
| as a keyword in this context and as an identifier elsewhere (although $(D do) is preferred).) ) |
There was a problem hiding this comment.
Is it just me, or there is one more ) here than there should be?
There was a problem hiding this comment.
I put the parens around "(Before the acceptance of... elsewhere (although do is preferred).) )"
The last parens are to close the paragraph $(P ...). I'll remove the "although do" set of parens.
This file did pass DAutoTest before the grammar commits, so it's not the cause of the errors.
There was a problem hiding this comment.
vim says the parens are legit.
Yes, it succeeds. The failed build fails about halfway through. Search for "LaTeX Warning: Hyper reference `MAIN' on page" on the successful build, and you'll see it appears halfway through, whereas that's the line that caused the failed build to fail. Baffling! Anyway, I made two tiny edits and now have to wait again on the results. |
|
Why don't you build it locally and see where the error is coming from? |
I couldn't build it. See the first comment in this PR: #2339 (comment) Also, the error, as it turns out, is in PDF generation, which I would have had to rely on the autotester for anyway. |
This is hard for me, because I couldn't get it to build on my machine, so I was forced to use DAutoTest. The instructions in CONTRIBUTING.md failed when "make -f posix.mak html" errored with: |
|
(EDIT: Sorry, shot in the dark. Turns out Ddoc allows nested parens in macros. Now I know...) I remembered seeing something odd, i.e. naked '( )' in the existing class and struct invariant grammars, instead of |
There is one with |
Should be fixed by #2340 |
I could be doing something wrong, but with that pull it still isn't reading the dub.sdl file for me: |
| $(GLINK MissingFunctionBody) | ||
|
|
||
| $(GNAME FunctionLiteralBody): | ||
| $(GLINK SpecifiedFunctionBody) |
There was a problem hiding this comment.
The build is failing because of this line.
It gets translated to this
{\fontshape{sl}\selectfont FunctionBody}:
\textit{SpecifiedFunctionBody}
\textit{MissingFunctionBody}
\par
{\fontshape{sl}\selectfont FunctionLiteralBody}:
\textit{SpecifiedFunctionBody}
\par
I don't know why removing these two line works though.
There was a problem hiding this comment.
Thanks for testing. I'm out of my depth on this, I'm afraid.
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
@CyberShadow think you can take a look at the LaTeX issue?
|
If it's another crazy problem with paging or whatever that causes it to break on a completely innocent-looking spot, then I don't think there's much I can do rather than suggest to drop PDF generation, or switch to a different method to generate it, like rendering+printing HTML in a headless browser. |
|
I think the failure is related to this #2363 (comment)
https://tex.stackexchange.com/questions/229113/another-extra-or-forgotten-endgroup However, I don't know how to fix it either. Probably breaking the Grammar sections into individual pieces could help... |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Please don't merge this. In case you didn't see the issue yet : everybody have missed that InExpression existed before DIP 1009 (https://issues.dlang.org/show_bug.cgi?id=19062).
| $(GLINK OutStatement) | ||
|
|
||
| $(GNAME InExpression): | ||
| $(D in $(LPAREN)) $(GLINK2 expression, ContractArguments) $(D $(RPAREN)) |
There was a problem hiding this comment.
InExpression already exists [1].
So this makes it compile: diff --git a/spec/function.dd b/spec/function.dd
index 685d597e..1b7b10e8 100644
--- a/spec/function.dd
+++ b/spec/function.dd
@@ -52,7 +52,11 @@ $(GNAME InOutX):
$(D ref)
$(RELATIVE_LINK2 return-ref-parameters, $(D return ref))
$(D scope)
+)
+
+$(H4 $(LNAME2 grammar, foo))
+$(GRAMMAR
$(GNAME FunctionAttributes):
$(GLINK FunctionAttribute)
$(GLINK FunctionAttribute) $(I FunctionAttributes)So as mentioned just splitting up the grammar section in multiple pieces... |
|
It passes. That is all that matters at this point. Thank you. |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Made my pass. @zachthemystic it has 12 commits now. Should we squash them?
| contracts validate the result of the statement. The most typical use of this | ||
| would be in validating the return value of a function and of any side effects it has. | ||
| The syntax is:) | ||
| In D, pre contracts begin with `in`, and post contracts begin with `out`. They |
There was a problem hiding this comment.
Not sure about this, but I would spell them pre-contracts and post-contracts? (Google even suggest precontract)
There was a problem hiding this comment.
I just copied the style of the previous documentation. It seems subjective, and not a source of confusion.
spec/contracts.dd
Outdated
|
|
||
| $(P As of the acceptance of | ||
| $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md, DIP1009), | ||
| pre and post contracts can be written either in expression form, with |
There was a problem hiding this comment.
Not sure whether we should mention the DIP in the specification as they are supposed to be self-contained.
Hence, I would simply say: "Pre- and post-contracts can be written ..."
There was a problem hiding this comment.
It should probably just indicate the first compiler version that incorporates the feature then. ( 2.081.0 )
| in(a > 0) | ||
| in(b >= 0, "b cannot be negative!") | ||
| out(r; r > 0, "return must be positive") | ||
| out(; a != 0) |
There was a problem hiding this comment.
FWIW D/Phobos style is to indent on the same level: https://dlang.org/dstyle.html#phobos_declarations
| $(P Invariant blocks should contain `assert` expressions, and should throw | ||
| `AssertError`s when they fail. As of | ||
| $(LINK2 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md, DIP1009), | ||
| invariants can also be written as expression statements, with `assert` implied: |
There was a problem hiding this comment.
Again, I'm not sure we should mention the DIP in a specification document that is intended to be read offline.
| $(GNAME MissingFunctionBody): | ||
| $(D ;) | ||
| $(GLINK FunctionContracts)$(OPT) $(GLINK InOutContractExpression) $(D ;) | ||
| $(GLINK FunctionContracts)$(OPT) $(GLINK InOutStatement) |
There was a problem hiding this comment.
Isn't there a semi-colon missing?
There was a problem hiding this comment.
No. The existing grammar is peculiar, and possibly bad design, but it is meant to prevent a semicolon coming after the end brace, };, as in:
int func() in { ... }; // <-- bad, already an error
The way you indicate the body here would be to now say do. Anything else (other than another in or out) means the function body is missing, and the declaration is over, and the parser moves on.
Arguably the no-semicolon-after-brace design is questionable. It hasn't ever mattered much, because currently a contact without a body is only legal in interface declarations, and rarely appears. It could however be expanded to ordinary function declarations eventually, at which point we might see more complaints about it. (e.g. "Why can't I just add the semicolon?")
Anyway, after a parenthesized contract with no body, the expectation would be for the semicolon to be there:
int func() in(...); // <-- legal and expected
There was a problem hiding this comment.
Thanks a lot for the extensive explanation.
For the record, I do think this will be confusing for users if contracts are ever allowed for declaration-only functions/structs/classes:
int func() in(...); // required
int func() in(...); // error
But yeah that's DMD's current behaviour: https://run.dlang.io/is/UwxsE0
|
|
||
| $(GNAME FunctionContract): | ||
| $(GLINK InOutContractExpression) | ||
| $(GLINK InOutStatement) |
There was a problem hiding this comment.
Couldn't you group all of them under FunctionContract?
FunctionContract:
- InContractExpression
- OutContractExpression
- InStatement
- OutStatement
This would also save you two lines above.
There was a problem hiding this comment.
There might be a more concise way to write it. The reason they are separate is the need to write do between an In/OutStatement and the function body, whereas there is no need for In/OutExpressions. Timon Gehr wrote it, and he is generally very meticulous, so I left it as is, although I sympathize with the desire to be more concise.
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
So a squash and we can finally merge this?
wilzbach
dismissed
jacob-carlborg’s stale review
In/OutExpression ~> In/OutContractExpression
|
One last concern. Maybe we should squash into two commits, with the split being at where the grammar finally passed the pdf generation, for future reference? |
|
Good idea. |
zachthemystic
force-pushed
the
patch-1
branch
2 times, most recently
from
781d9c4 to
2a71bb0
Compare
|
Alright, I don't want my poor git skills to delay it. If you can reduce it to 2 commits do it. It currently pulls the master branch into it as the 2nd to last commit. Sorry about my git skills. |
wilzbach
force-pushed
the
patch-1
branch
2 times, most recently
from
26193de to
6c5b4f1
Compare
|
No worries. Fixed it for you ;-) |
|
Thanks :) |
7 participants
I edited this on github, but then was unable to compile the docs it to check its final appearance in html (See here for why: https://forum.dlang.org/post/azdfcrfqegubikcyntfj@forum.dlang.org ). At any rate, I'm hoping this can be looked at, in particular by @tgehr for accuracy.
Also, the grammar spec is in a different file and still needs to be changed.