Prevent empty statements at the end of --eval or --loop code by WebDrake · Pull Request #303 · dlang/tools

WebDrake · 2018-02-02T23:48:16Z

The --eval and --loop flags allow the user to specify small snippets of code that are placed inside the body of an auto-generated main that is written to a file and evaluated.

The existing implementation uses two separate string concatenations for the two different cases. Besides the code duplication, this means that the resulting code generation cannot be tested.

A further more subtle issue arises from the existing implementation's insistence on adding a closing ; to the generated code. While this takes care of the careless user who forgets to write one themselves, it means that if the user does close their statements correctly, we end up with an empty statement on the end of the code to be evaluated. While some D compilers don't care about this, some object: #297 (comment)

... meaning that rdmd will fail to successfully evaluate the code.

These patches rework the implementation around a new makeEvalCode function taking two parameters: the string[] of code snippets to be evaluated, and a boolean flag indicating whether or not the code should be treated as the body of a loop (in other words, whether the code snippets derived from an --eval or a --loop flag).

Generation of the innermost code (the common part for both --eval and --loop options) is implemented in a second new function, innerEvalCode, which takes care of joining the different entries in the string[], appending a terminating ; only if one is not already present. In consequence, the generated code should never end in a blank statement (at least, not unless it was explicitly put there by the user).

Unittests have been provided with both the new functions to ensure that they produce the expected outcomes. The first patch provides the functions themselves; the second uses them to replace the old implementation.

dlang-bot · 2018-02-02T23:48:17Z

Thanks for your pull request, @WebDrake! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

WebDrake · 2018-02-02T23:52:25Z

For reference: the new function should produce identical results to the old ad-hoc implementations, apart from the trailing ;.

WebDrake · 2018-02-03T00:39:55Z

rdmd.d

+    string code = eval.join("\n");
+    if (stripRight(eval[$ - 1])[$ - 1] != ';')
+        code ~= ';';
+    yap("[inner-code]\n", code);


Oh, whoops. This shouldn't be there, it was a debugging check. I'll remove.

WebDrake · 2018-02-03T00:42:54Z

Removed the unintended debug statement:

diff --git a/rdmd.d b/rdmd.d
index d87ac76..118471c 100755
--- a/rdmd.d
+++ b/rdmd.d
@@ -855,7 +855,6 @@ string innerEvalCode(string[] eval)
     string code = eval.join("\n");
     if (stripRight(eval[$ - 1])[$ - 1] != ';')
         code ~= ';';
-    yap("[inner-code]\n", code);
     return code;
 }

marler8997

Other than comments, looks good.

marler8997 · 2018-02-03T07:16:37Z

rdmd.d

+        to the inner code of either the program or
+        the loop
+*/
+string makeEvalCode(string[] eval, bool loop = false)


Flag!"loop" instead?

marler8997 · 2018-02-03T07:18:36Z

rdmd.d

+{
+    import std.string: join, stripRight;
+    string code = eval.join("\n");
+    if (stripRight(eval[$ - 1])[$ - 1] != ';')


Should you handle the case where eval.length == 0, or the case where eval[$-1].length == 0 rather than causing an overflow?

marler8997 · 2018-02-03T07:20:37Z

rdmd.d

 ";

+/**
+    Joins together the code provided via an `--eval`


I believe the agreed upon convention is to have the ddoc content aligned with the comment, which in this case means no leading whitespace.

Yes, please follow the DStyle for new code.

marler8997 · 2018-02-03T07:21:58Z

rdmd.d

                "program file ('" ~ args[programPos] ~ "').");
-        root = makeEvalFile(importWorld ~ "void main(char[][] args) {\n"
-                ~ std.string.join(eval, "\n") ~ ";\n}");
+        root = makeEvalFile(makeEvalCode(eval));


makeEvalCode(eval, No.loop) seems better

marler8997 · 2018-02-03T07:22:08Z

rdmd.d

-                ~ "foreach (line; std.stdio.stdin.byLine()) {\n"
-                ~ std.string.join(loop, "\n")
-                ~ ";\n} }");
+        root = makeEvalFile(makeEvalCode(loop, true));


makeEvalCode(eval, Yes.loop) seems better

wilzbach · 2018-02-03T10:10:00Z

rdmd.d

+*/
+string innerEvalCode(string[] eval)
+{
+    import std.string: join, stripRight;


Nit: space before and after : for selective imports (DStyle for official code).

wilzbach · 2018-02-03T10:11:04Z

rdmd.d

+        ~ "void main(char[][] args) {%s%s\n%s}";
+
+    immutable innerCodeOpening =
+        loop ? " foreach (line; std.stdio.stdin.byLine()) {\n"


Idiomatic D would just use .byLine - you could use this opportunity to change this bit.

wilzbach · 2018-02-03T10:11:49Z

rdmd.d

+{
+    import std.format : format;
+    immutable codeFormat = importWorld
+        ~ "void main(char[][] args) {%s%s\n%s}";


edit: DMD also emits the same in the default main method: https://github.com/dlang/dmd/blob/master/src/dmd/mars.d#L130

wilzbach · 2018-02-03T10:13:37Z

rdmd.d

+                  innerCodeClosing);
+}
+
+unittest


Could be @safe

@safe won't go with the use of assumeSafeAppend inside innerEvalCode. That might be possible to drop, but OTOH, I'm not sure I care to overly restrict how anyone edits or revises these functions in future. It's not like rdmd.d makes common use of these kinds of constraints.

WebDrake · 2018-02-03T11:07:55Z

@wilzbach I don't think it makes sense to bundle changes to how the code is generated, with changes to what code is generated (beyond the base goal of fixing the trailing-empty-statement issue).

I don't want to assume that there were not deliberate motivations behind the choices made here, and I don't want to risk breaking anyone's use-case by changing things that do not need to be changed.

The `--eval` and `--loop` flags allow the user to specify small snippets of code that are placed inside the body of an auto-generated `main` that is written to a file and evaluated. The existing implementation uses two separate string concatenations for the two different cases. Besides the code duplication, this means that the resulting code generation cannot be tested. A further more subtle issue arises from the existing implementation's insistence on adding a closing `;` to the generated code. While this takes care of the careless user who forgets to write one themselves, it means that if the user does close their statements correctly, we end up with an empty statement on the end of the code to be evaluated. While some D compilers don't care about this, some object: dlang#297 (comment) ... meaning that `rdmd` will fail to successfully evaluate the code. This patch therefore implements two new code functions to generate the required code. The first, `innerEvalCode`, takes care of the innermost code (the body of either the `main` function for `--eval`, or the loop for `--loop`), adding a terminating `;` if one is not present, but not doing so if one is already in place. The second, `makeEvalCode`, takes two parameters: the `string[]` of code snippets to be evaluated, and a boolean `Flag` indicating whether or not this is the body of a loop (in other words, whether this code should be generated for the `--eval` flag or the `--loop` flag). Unittests have been provided with both to ensure that they produce the expected outcomes.

This patch replaces the old ad-hoc generation of `--eval` or `--loop` program code with calls to the new `makeEvalCode` function. Besides improving separation of concerns, this also ensures that the generated code will never have a trailing blank statement (i.e. two `;` separated at most by whitespace). This should prevent issues when using `--eval` or `--loop` with a `--compiler` choice that is fussy about such things; see dlang#297 (comment) for an example.

WebDrake · 2018-02-03T11:16:29Z

I've updated the ddoc, replaced the bool with a Flag!"loop" (good call @marler8997!), and reworked the internals of innerEvalCode to avoid the risk of overflow. A small by-product is that it now right-strips trailing whitespace from the generated code (meaning unittests have been updated slightly), but this makes for no practical difference in outcome.

Overall diff:

diff --git a/rdmd.d b/rdmd.d
index 118471c..250f3f2 100755
--- a/rdmd.d
+++ b/rdmd.d
@@ -17,7 +17,7 @@
 import std.algorithm, std.array, core.stdc.stdlib, std.datetime,
     std.digest.md, std.exception, std.file, std.getopt,
     std.parallelism, std.path, std.process, std.range, std.regex,
-    std.stdio, std.string, std.typetuple;
+    std.stdio, std.string, std.typecons, std.typetuple;
 
 version (Posix)
 {
@@ -180,14 +180,14 @@ int main(string[] args)
     {
         enforce(programPos == args.length, "Cannot have both --loop and a " ~
                 "program file ('" ~ args[programPos] ~ "').");
-        root = makeEvalFile(makeEvalCode(loop, true));
+        root = makeEvalFile(makeEvalCode(loop, Yes.loop));
         argsBeforeProgram ~= "-d";
     }
     else if (eval.ptr)
     {
         enforce(programPos == args.length, "Cannot have both --eval and a " ~
                 "program file ('" ~ args[programPos] ~ "').");
-        root = makeEvalFile(makeEvalCode(eval));
+        root = makeEvalFile(makeEvalCode(eval, No.loop));
         argsBeforeProgram ~= "-d";
     }
     else if (programPos < args.length)
@@ -836,24 +836,24 @@ import std.stdio, std.algorithm, std.array, std.ascii, std.base64,
 ";
 
 /**
-    Joins together the code provided via an `--eval`
-    or `--loop` flag, ensuring a trailing `;` is
-    added if not already provided by the user
-
-    Params:
-        eval = array of strings generated by the
-               `--eval` or `--loop` rdmd flags
-
-    Returns:
-        string of code to be evaluated, corresponding
-        to the inner code of either the program or
-        the loop
+Joins together the code provided via an `--eval` or `--loop`
+flag, ensuring a trailing `;` is added if not already provided
+by the user
+
+Params:
+    eval = array of strings generated by the `--eval`
+           or `--loop` rdmd flags
+
+Returns:
+    string of code to be evaluated, corresponding to the
+    inner code of either the program or the loop
 */
 string innerEvalCode(string[] eval)
 {
-    import std.string: join, stripRight;
-    string code = eval.join("\n");
-    if (stripRight(eval[$ - 1])[$ - 1] != ';')
+    import std.string : join, stripRight;
+    // assumeSafeAppend just to avoid unnecessary reallocation
+    string code = eval.join("\n").stripRight.assumeSafeAppend;
+    if (code.length > 0 && code[$ - 1] != ';')
         code ~= ';';
     return code;
 }
@@ -864,34 +864,33 @@ unittest
     assert(innerEvalCode([`writeln("Hello!");`]) == `writeln("Hello!");`);
 
     // test with trailing whitespace
-    assert(innerEvalCode([`writeln("Hello!")  `]) == `writeln("Hello!")  ;`);
-    assert(innerEvalCode([`writeln("Hello!");  `]) == `writeln("Hello!");  `);
+    assert(innerEvalCode([`writeln("Hello!")  `]) == `writeln("Hello!");`);
+    assert(innerEvalCode([`writeln("Hello!");  `]) == `writeln("Hello!");`);
 
     // test with multiple entries
     assert(innerEvalCode([`writeln("Hello!");  `, `writeln("You!")  `])
-           == "writeln(\"Hello!\");  \nwriteln(\"You!\")  ;");
+           == "writeln(\"Hello!\");  \nwriteln(\"You!\");");
     assert(innerEvalCode([`writeln("Hello!");  `, `writeln("You!"); `])
-           == "writeln(\"Hello!\");  \nwriteln(\"You!\"); ");
+           == "writeln(\"Hello!\");  \nwriteln(\"You!\");");
 }
 
 /**
-    Formats the code provided via `--eval` or `--loop`
-    flags into a string of complete program code that
-    can be written to a file and then compiled
-
-    Params:
-        eval = array of strings generated by the
-               `--eval` or `--loop` rdmd flags
-        loop = set to `true` if this code comes
-               from a `--loop` flag, `false` (the
-               default) otherwise
-
-    Returns:
-        string of code to be evaluated, corresponding
-        to the inner code of either the program or
-        the loop
+Formats the code provided via `--eval` or `--loop` flags into a
+string of complete program code that can be written to a file
+and then compiled
+
+Params:
+    eval = array of strings generated by the `--eval` or
+           `--loop` rdmd flags
+    loop = set to `Yes.loop` if this code comes from a
+           `--loop` flag, `No.loop` if it comes from an
+           `--eval` flag
+
+Returns:
+    string of code to be evaluated, corresponding to the
+    inner code of either the program or the loop
 */
-string makeEvalCode(string[] eval, bool loop = false)
+string makeEvalCode(string[] eval, Flag!"loop" loop)
 {
     import std.format : format;
     immutable codeFormat = importWorld
@@ -914,21 +913,21 @@ unittest
     // innerEvalCode already tests the cases for different
     // contents in `eval` array, so let's focus on testing
     // the difference based on the `loop` flag
-    assert(makeEvalCode([`writeln("Hello!") `]) ==
+    assert(makeEvalCode([`writeln("Hello!") `], No.loop) ==
            importWorld
            ~ "void main(char[][] args) {\n"
-           ~ "writeln(\"Hello!\") ;\n}");
+           ~ "writeln(\"Hello!\");\n}");
 
-    assert(makeEvalCode([`writeln("What!"); `], false) ==
+    assert(makeEvalCode([`writeln("What!"); `], No.loop) ==
            importWorld
            ~ "void main(char[][] args) {\n"
-           ~ "writeln(\"What!\"); \n}");
+           ~ "writeln(\"What!\");\n}");
 
-    assert(makeEvalCode([`writeln("Loop!") ; `], true) ==
+    assert(makeEvalCode([`writeln("Loop!") ; `], Yes.loop) ==
            importWorld
            ~ "void main(char[][] args) { "
            ~ "foreach (line; std.stdio.stdin.byLine()) {\n"
-           ~ "writeln(\"Loop!\") ; \n} }");
+           ~ "writeln(\"Loop!\") ;\n} }");
 }
 
 string makeEvalFile(string todo)

wilzbach · 2018-02-03T11:18:00Z

I don't want to assume that there were not deliberate motivations behind the choices made here, and I don't want to risk breaking anyone's use-case by changing things that do not need to be changed.

Fair enough,

Overall diff:

Are you aware of these two handy features from GH?

https://github.com/dlang/tools/pull/303.diff
#303

WebDrake · 2018-02-03T11:21:06Z

Are you aware of these two handy features from GH?

Nice, but if I understand correctly, that's the overall diff of the PR. I posted the diff with between the original and versions of the PR. ;-)

WebDrake · 2018-02-03T18:31:51Z

Ping on this? Is there anything that remains to do (e.g. a changelog entry or Bugzilla issue)?

This patch is something of a proof-of-concept for using rdmd_test's new `--test-compilers` flag to test rdmd using multiple different compilers. The `install.sh` script is used to install GDC and LDC (including the `gdmd` and `ldmd2` DMD-alike compiler interfaces), and specify all three of `dmd`, `gdmd` and `ldmd2` with the `RDMD_TEST_COMPILERS` environment variable when invoking `make -f posix.mak test`. A little trickery with `find` is used to work around the problem that the install locations of `gdmd` and` ldmd2` are not added to `PATH`. The `VERBOSE_RDMD_TEST` flag is set to ensure that the Travis build logs will contain a full summary of the individual `rdmd` calls from the test suite, for each of the test compilers. This should probably not be the final form for setting up multi compiler tests, but serves as an initial stage which should at a minimum flag up issues like that observed with dlang#271. While in development it has already shown up at least one `rdmd` issue (fixed in dlang#303), so we can reasonably hope that it will prevent others from arising in the first place.

This patch is something of a proof-of-concept for using rdmd_test's new `--test-compilers` flag to test rdmd using multiple different compilers. The `install.sh` script is used to install LDC (including the DMD-alike `ldmd2` compiler interface interface), and the `make -f posix.mak test` call is updated to specify both `dmd` and `ldmd2` as test compilers for the `rdmd_test` test suite. A little trickery with `find` is necessary to work around the problem that the install location of` ldmd2` is not added to `PATH`. The `VERBOSE_RDMD_TEST` flag is set to ensure that the Travis build logs will contain a full summary of the individual `rdmd` calls from the test suite, for each of the test compilers. This should probably not be the final form for setting up multi compiler tests, but serves as an initial stage which should at a minimum flag up issues like that observed with dlang#271. While in development it has already shown up at least one `rdmd` issue (fixed in dlang#303), so we can reasonably hope that it will prevent others from arising in the first place.

Partially revert #303 until #317 is resolved merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

Revert "Partially revert #303 until #317 is resolved" merged-on-behalf-of: Vladimir Panteleev <github@thecybershadow.net>

WebDrake mentioned this pull request Feb 2, 2018

Use multiple different test compilers when invoking rdmd_test from Travis CI #297

Merged

WebDrake commented Feb 3, 2018

View reviewed changes

WebDrake force-pushed the prevent-eval-loop-empty-statements branch from 90d5f45 to 41be9b0 Compare February 3, 2018 00:42

marler8997 approved these changes Feb 3, 2018

View reviewed changes

wilzbach reviewed Feb 3, 2018

View reviewed changes

WebDrake added 2 commits February 3, 2018 12:08

WebDrake force-pushed the prevent-eval-loop-empty-statements branch from 41be9b0 to 1a29cdb Compare February 3, 2018 11:12

marler8997 approved these changes Feb 3, 2018

View reviewed changes

wilzbach approved these changes Feb 4, 2018

View reviewed changes

wilzbach merged commit a8d282e into dlang:master Feb 4, 2018

wilzbach mentioned this pull request Feb 13, 2018

rdmd: and with a last statement without effect should print it #317

Open

wilzbach added a commit to wilzbach/tools that referenced this pull request Feb 13, 2018

Partially revert dlang#303 until dlang#317 is resolved

6a1b3fb

wilzbach mentioned this pull request Feb 13, 2018

Partially revert #303 until #317 is resolved #318

Merged

dlang-bot added a commit that referenced this pull request Feb 13, 2018

Merge pull request #318 from wilzbach/partially-revert-303

5cc9864

Partially revert #303 until #317 is resolved merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

wilzbach added a commit that referenced this pull request Feb 13, 2018

Revert "Partially revert #303 until #317 is resolved"

40ef433

wilzbach mentioned this pull request Feb 13, 2018

Revert "Partially revert #303 until #317 is resolved" #319

Merged

dlang-bot added a commit that referenced this pull request Feb 13, 2018

Merge pull request #319 from dlang/revert-318-partially-revert-303

255dc4d

Revert "Partially revert #303 until #317 is resolved" merged-on-behalf-of: Vladimir Panteleev <github@thecybershadow.net>

Uh oh!

Conversation

WebDrake commented Feb 2, 2018

Uh oh!

dlang-bot commented Feb 2, 2018

Bugzilla references

Uh oh!

WebDrake commented Feb 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebDrake commented Feb 3, 2018

Uh oh!

marler8997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebDrake commented Feb 3, 2018

Uh oh!

WebDrake commented Feb 3, 2018

Uh oh!

wilzbach commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WebDrake commented Feb 3, 2018

Uh oh!

WebDrake commented Feb 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wilzbach Feb 3, 2018 •

edited

Loading

wilzbach commented Feb 3, 2018 •

edited

Loading