Skip to content

Fix emacs state corruption from incorrect "syntax-begin-function" setting #38

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

Merged
merged 2 commits into from
Feb 16, 2015

Conversation

MicahChalmer
Copy link
Contributor

This is to fix #36.

Commit 6b57bbf set syntax-begin-function to beginning-of-defun. This violated the contract for that function--it's supposed to get outside of all braces/parens--but functions could appear inside other blocks, so beginning-of-defun could end inside another pair of braces. This in turn caused emacs corrupt some unknown internal state and cause itself to think there were fewer parens than there were.

The comments of the added test show how much I understood about the exact mechanism of the failure--which is not much. I figure it's better to just fix it and leave the test there. Though I don't understand the mechanism of the corruption, at least I understand how the code was violating the hook's contract. Hopefully there isn't some other similar corruption hiding out that I haven't found yet.

@vhbit and @hugoduncan, does this fix the issues you were seeing?

@rust-highfive
Copy link

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

could leave it inside parens if a fn appears inside them.

Having said that, as I write this I don't understand fully what
internal state was corruped and how. There wasn't an obvious
Copy link
Member

Choose a reason for hiding this comment

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

typo: "corruped"

@pnkfelix
Copy link
Member

I have nits, but the fix seems as sound as far I understand it. So I'm going to land this PR, and file an issue asking @MicahChalmer to fix the comments. :)

@pnkfelix
Copy link
Member

Oh, one more question: @MicahChalmer can you elaborate (or point to an elaboration elsewhere) on why #30 included that setq of syntax-begin-function to beginning-of-defun ? I.e. do you remember if there was a specific bug that you were addressing by doing that?

pnkfelix added a commit that referenced this pull request Feb 16, 2015
Fix emacs state corruption from incorrect "syntax-begin-function" setting
@pnkfelix pnkfelix merged commit 97f6445 into rust-lang:master Feb 16, 2015
@MicahChalmer MicahChalmer deleted the fix-issue-36 branch February 16, 2015 01:36
@MicahChalmer
Copy link
Contributor Author

I filed #40 to fix the typo and missing end of the comment.

As to @pnkfelix's question of why I made the mistake of setting syntax-begin-function in #30 in the first place...when I wrote #30 I was trying to translate from the previous approach of using syntax-propertize-function and syntax-propertize-extend-region. I was under the mistaken impression that syntax-begin-function was used in a similar way to the first element of the return value syntax-propertize-extend-region--that is, I thought it was used to start further back and thus avoid starting intepretation in the middle of a construct. But it's actually used to optimize--if syntax-begin-function is defined, it's only used to start further forward than the default, rather than further back. It's only there to make it go faster. (There's a check in syntax.el that ignores the return value if it's further back than the default.) Unless there are performance problems (and nobody has reported any so far) there is no reason to set it at all.

Of course, since I was also the one who put in the syntax-propertize-extend-region implementation as well, this naturally leads to follow-up questions of whether that was necessary (and why I thought it was.) I don't remember the answers to these.

I didn't put the above in the comments in #40--I figured it's a bit more than needed for ongoing maintenance of the code. Let me know if you think it should go in anyway, and I can add it as another commit to #40.

@vhbit
Copy link

vhbit commented Feb 16, 2015

@MicahChalmer I haven't chance to work a lot on Rust today, but on a quick look everything was fine, thanks!

@hugoduncan
Copy link

It seems to have fixed things for me. Many thanks for tracking that down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impl fn indented to left margin
6 participants