Tutorial doc #13676

mdinger · 2014-04-22T05:27:39Z

Improve tutorial discussion of closures, e.g. with respect to type inference and variable capture.

---- original description follows

I'd like this pulled to master if possible but if not I'd appreciate comments on what I need to change. I found the closures difficult to understand as they were so I tried to explain it so I would've had an easier time understanding it. I think it's better at least, somewhat.

I don't know that everyone liked the -> () I included but I thought explicit is best to aid understanding. I thought it was much harder to understand than it should have been.

[EDIT] - Clicked too early.
This doesn't make check without errors on my Xubuntu on Virtualbox machine. Not sure why. I don't think I changed anything problematic. I'll try make check on master tomorrow.

Opened #13621 regarding this.

mdinger · 2014-04-22T05:43:16Z

It touches guide-tasks also because I didn't think it was worth pulling into a different branch. I don't know if you will care.

adrientetar · 2014-04-22T11:23:06Z

src/doc/tutorial.md

-annotated.
+Since a closure is an expression, the compiler can usually infer the argument and
+return types; so they are often omitted.  This is in contrast to a function which
+is a declaration and _not_ an expression.  Declarations require the types to be


nit: do not like the stacked spaces here – this is not a proper way of justifying text (would be done in the frontend if needed).

mdinger · 2014-04-22T16:25:51Z

Fixed nits and added {.ignore} to the 2 failing code blocks.

mdinger · 2014-04-22T17:34:43Z

Travis CI stalled. Can you please restart it?

[EDIT] Nix that. Sorry about double build. Just saw Travis CI has a queue.

mdinger · 2014-04-23T13:00:10Z

Could you please review this again?

pnkfelix · 2014-04-30T12:56:42Z

src/doc/tutorial.md

+
+// `fun_arg` is an invalid definition
+fn  fun_arg       (arg: int) -> () { println!("{}", arg + x) }; // cannot capture enclosing scope
+let closure_arg = |arg: int| -> () { println!("{}", arg + x) }; // Can capture enclosing scope


any reason you capitalized "Can" here, while using lowercase elsewhere?

No, I'll fix it.

pnkfelix · 2014-04-30T13:11:32Z

src/doc/tutorial.md

+
+~~~~ {.ignore}
+// `fun` cannot infer the type of `x` so it must be provided because it is a function.
+fn  fun       (x: int) -> () { println!("{}", x) };


The semicolon at the end of this fn item is unnecessary, and including it probably violates some style guideline somewhere. (I'm a little surprised this actually parses.)

(As opposed to semi-colon at the end of the let closure = ...;, which is necessary.)

pnkfelix · 2014-04-30T13:17:31Z

I don't have any serious problem with the spirit of this change. (at points during its development I wondered if I would prefer to just revise the documentation here more completely, but I don't want to take the time to do a proper job of that.)

So if you could incorporate the feedback I have provided, then I'll put it through another round of review (and hopefully eventually mark it for incorporation onto master).

pnkfelix · 2014-04-30T14:54:32Z

src/doc/tutorial.md

+closure(20); // Prints 20
+
+fun("String"); // Error: wrong type
+// Error: This type is different from when `x` was originally evaluated


I think this would be better phrased as "closure has already been assigned the type |int| -> () via inference." (I'm planning on providing @mdinger with a pull request which will show him another phrasing.)

Most important: distinguish function decl sugar for omitting `-> ()` from type inference on closures. I also tried to add a couple more examples to further emphasize this distinction. Note that this sugar (of omitting `-> ()`) is actually already briefly mentioned in an earlier section, so it is a little tricky deciding whether to put more material here, or to move it up to the previous section. Other drive-by fixes: * Fix the line length of the code blocks to fit in the width provided in the rendered HTML * Some minor revisions to wording (e.g. try to clarify in some cases where a type mismatch is arising).

pnkfelix · 2014-04-30T16:39:56Z

@mdinger I attempted to open a PR against your repository, but for some reason github's interface is not letting me select your repository. (I would have expected you to show up between mcandre/rust and megakorre/rust in the pop-up list, but it is not there. Maybe it is too new? Or you need to flip some switch on your repository?)

Anyway, if you want to see my changes (or even just pull them into your own repository), you can see the commit here: pnkfelix@636f7d2

Suggested revisions to PR 13676.

mdinger · 2014-05-01T01:23:28Z

Note that this sugar
(of omitting -> ()) is actually already briefly mentioned in an
earlier section, so it is a little tricky deciding whether to put more
material here, or to move it up to the previous section.

pnkfelix/rust@636f7d2

I didn't think the concept of returning unit from the functions section was difficult but I did think it important to contrast the function and closure syntax directly. Then it makes more sense to me to discuss it in closures where it is directly relevant. In the functions section, the distinction may not matter yet.

mdinger · 2014-05-01T01:36:28Z

Regarding pnkfelix/rust@636f7d2:
I'm quite happy with your changes. They seem to explain declarations vs expressions more precisely than I had achieved. Your comments are also more specific about reasons for errors.

thestinger · 2014-05-04T20:37:32Z

src/doc/guide-tasks.md

@@ -101,6 +101,8 @@ fn print_message() { println!("I am running in a different task!"); }
 spawn(print_message);

 // Print something more profound in a different task using a lambda expression
+// This uses the proc() keyword to assign to spawn a function with no name


I don't think the term assign is great here since it's a normal parameter being passed.

The language there is poor in a number of respects, sorry I missed that earlier.

(filed as PR #13946.)

Improve tutorial discussion of closures, e.g. with respect to type inference and variable capture. Fix #13621 ---- original description follows I'd like this pulled to master if possible but if not I'd appreciate comments on what I need to change. I found the closures difficult to understand as they were so I tried to explain it so I would've had an easier time understanding it. I think it's better at least, somewhat. I don't know that everyone liked the `-> ()` I included but I thought explicit is best to aid understanding. I thought it was much harder to understand than it should have been. [EDIT] - Clicked too early. This doesn't `make check` without errors on my Xubuntu on Virtualbox machine. Not sure why. I don't think I changed anything problematic. I'll try `make check` on master tomorrow. Opened #13621 regarding this.

…e-tasks, r=alexcrichton Followup to some recent feedback in PR #13676.

Mega-sync from `rust-lang/rust` This essentially implements `@oli-obk's` suggestion here rust-lang/rust-analyzer#13459 (comment), with `@eddyb's` help. This PR is equivalent to 14 syncs (back and forth) between `rust-lang/rust` and `rust-lang/rust-analyzer`. Working from this list (from bottom to top): ``` (x) a2a1d99 ⬆️ rust-analyzer (x) 79923c3 ⬆️ rust-analyzer (x) c60b1f6 ⬆️ rust-analyzer (x) 8807fc4 ⬆️ rust-analyzer (x) a99a48e ⬆️ rust-analyzer (x) 4f55ebb ⬆️ rust-analyzer (x) f5fde4d ⬆️ rust-analyzer (x) 459bbb4 ⬆️ rust-analyzer (x) 65e1dc4 ⬆️ rust-analyzer (x) 3e358a6 ⬆️ rust-analyzer (x) 31519bb ⬆️ rust-analyzer (x) 8231fee ⬆️ rust-analyzer (x) 22c8c9c ⬆️ rust-analyzer (x) 9d2cb42 ⬆️ rust-analyzer ``` (This listed was assembled by doing a `git subtree push`, which made a branch, and looking at the new commits in that branch, picking only those that were `⬆️ rust-analyzer` commits) We used the following commands to simulate merges in both directions: ```shell TO_MERGE=22c8c9c40 # taken from the list above, bottom to top git merge --no-edit --no-ff $TO_MERGE git merge --no-edit --no-ff $(git -C ../rust log --pretty=format:'%cN | %s | %ad => %P' | rg -m1 -F "$(git show --no-patch --pretty=format:%ad $TO_MERGE)" | tee /dev/stderr | rg '.* => \S+ (\S+)$' --replace '$1') ``` We encountered no merge conflicts that Git wasn't able to solve by doing it this way. Here's what the commit graph looks like (as shown in the Git Lens VSCode extension): <img width="1345" alt="image" src="https://user-images.githubusercontent.com/7998310/203984523-7c1a690a-8224-416c-8015-ed6e49667066.png"> This PR closes rust-lang#13459 ## Does this unbreak `rust->ra` syncs? Yes, here's how we tried: In `rust-analyzer`: * check out `subtree-fix` (this PR's branch) * make a new branch off of it: `git checkout -b subtree-fix-merge-test` * simulate this PR getting merged with `git merge master` In `rust`: * pull latest master * make a new branch: `git checkout -b test-change` * mess with rust-analyzer (I added a comment to `src/tools/rust-analyzer/Cargo.toml`) * commit * run `git subtree push -P src/tools/rust-analyzer ra-local final-sync` (this follows the [Clippy sync guide](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html)) This created a `final-sync` branch in `rust-analyzer`. In `rust-analyzer`: * `git merge --no-ff final-sync` (this follows the [Clippy sync guide](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html)) Now `git log` in `rust-analyzer` shows this: ``` commit 460128387e46ddfc2b95921b2d7f6e913a3d2b9f (HEAD -> subtree-fix-merge-test) Merge: 0513fc02a 9ce6a734f Author: Amos Wenger <[email protected]> Date: Fri Nov 25 13:28:24 2022 +0100 Merge branch 'final-sync' into subtree-fix-merge-test commit 0513fc02a08ea9de952983624bd0a00e98044b36 Merge: 38c98d1 6918009 Author: Amos Wenger <[email protected]> Date: Fri Nov 25 13:28:02 2022 +0100 Merge branch 'master' into subtree-fix-merge-test commit 9ce6a734f37ef8e53689f1c6f427a9efafe846bd (final-sync) Author: Amos Wenger <[email protected]> Date: Fri Nov 25 13:26:26 2022 +0100 Mess with rust-analyzer just for fun ``` And `git diff 0513fc02a08ea9de952983624bd0a00e98044b36` shows this: ```patch diff --git a/Cargo.toml b/Cargo.toml index 286ef1e7d..c9e24cd19 100644 --- a/Cargo.toml +++ b/Cargo.toml `@@` -32,3 +32,5 `@@` debug = 0 # ungrammar = { path = "../ungrammar" } # salsa = { path = "../salsa" } + +# lol, hi ``` ## Does this unbreak `ra->rust` syncs? Yes, here's how we tried. From `rust`: * `git checkout -b sync-from-ra` * `git subtree pull -P src/tools/rust-analyzer ra-local subtree-fix-merge-test` (this is adapted from the [Clippy sync guide](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html#performing-the-sync-from-clippy-to-rust-langrust), you would normally use `ra-upstream master` but we're simulating things here) A commit editor pops up, there was no merge conflicts. ## How do we prevent this from happening again? Like `@bjorn3` said in rust-lang/rust-analyzer#13459 (comment) > Whenever syncing from rust-analyzer -> rust you have to immediately sync the merge commit from rust -> rust-analyzer to prevent merge conflicts in the future. But if we get it wrong again, at least now we have a not-so-painful way to fix it.

mdinger added 3 commits April 18, 2014 21:43

Be more clear about what this lambda expression does

ad0cdd7

Give more explanation when introducing closures

b580964

Demonstrate accessing external variable in first example

36f98fb

adrientetar reviewed Apr 22, 2014
View reviewed changes

Some more closure changes

f7d2d58

pnkfelix reviewed Apr 30, 2014
View reviewed changes

Fix typo

af0dd16

pnkfelix reviewed Apr 30, 2014
View reviewed changes

Remove extra semicolons

f79571f

pnkfelix reviewed Apr 30, 2014
View reviewed changes

Merge pull request #1 from pnkfelix/clarify-omit-unit-vs-inference

b3d7aa3

Suggested revisions to PR 13676.

thestinger reviewed May 4, 2014
View reviewed changes

bors closed this May 4, 2014

bors merged commit b3d7aa3 into rust-lang:master May 4, 2014

pnkfelix mentioned this pull request May 5, 2014

Cleanup proc comment in guide-tasks.md. #13946

Closed

bors added a commit that referenced this pull request May 6, 2014

auto merge of #13946 : pnkfelix/rust/fsk-cleanup-proc-comment-in-guid…

a1ca572

…e-tasks, r=alexcrichton Followup to some recent feedback in PR #13676.

mdinger deleted the tutorial_doc branch January 2, 2015 20:53

mdinger mentioned this pull request Mar 28, 2015

Re-write closures chapter #23568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tutorial doc #13676

Tutorial doc #13676

mdinger commented Apr 22, 2014

mdinger commented Apr 22, 2014

adrientetar Apr 22, 2014

mdinger commented Apr 22, 2014

mdinger commented Apr 22, 2014

mdinger commented Apr 23, 2014

pnkfelix Apr 30, 2014

mdinger Apr 30, 2014

pnkfelix Apr 30, 2014

pnkfelix commented Apr 30, 2014

pnkfelix Apr 30, 2014

pnkfelix commented Apr 30, 2014

mdinger commented May 1, 2014

mdinger commented May 1, 2014

thestinger May 4, 2014

pnkfelix May 5, 2014

pnkfelix May 5, 2014

Tutorial doc #13676

Tutorial doc #13676

Conversation

mdinger commented Apr 22, 2014

mdinger commented Apr 22, 2014

Choose a reason for hiding this comment

mdinger commented Apr 22, 2014

mdinger commented Apr 22, 2014

mdinger commented Apr 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Apr 30, 2014

Choose a reason for hiding this comment

pnkfelix commented Apr 30, 2014

mdinger commented May 1, 2014

mdinger commented May 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment