Replace rework with master

[theothornhill]

Rework is getting ready, and I think we are reaching the point where it fixes more bugs than it causes feature regressions. Maybe it is time to replace master?

What is missing:

• Imenu support - will not add
• Fontification inside attributes

What is working

• Much more fontification
• Faster in general
• Lexical scoping
• No fontification is patched with syntax-properties, so nothing will clash with CC Mode (for now )
• There is NO monkey-patch, but going to file a bug in emacs

We have a goal of getting this merged into emacs core, so that maintaining this will be tighter coupled with emacs itself.

What else is missing?

[josteink]

compilation-support has mostly been done by @jesse-black with some additional maintenance work done by me and one fix by @binki.

@binki & @jesse-black : Have you guys signed FSF copyright assignments? We're considering trying to hand this mode over back to core GNU Emacs, and need to know just how much of the existing code we have is compatible with their legal regime.

[theothornhill]

@josteink However, adding compilation support doesn't have to be a blocking factor in merging to master. If for some reason it can't be included in emacs we can remove it then.

Maybe we also skip imenu support altogether?

[josteink]

To be honest, compilation-mode support is something I use every single day I do C#. It's beyond useful. It's magic.

Leaving that out is IMO not an option.

imenu however, is OK for me to leave out, on the basis that it has largely been replaced by both lsp-mode or omnisharp-emacs. No need to write and maintain additional code for IDE like features, when there is already much better options for those who wants that experience. (Unless anyone else objects?)

[theothornhill]

Yeah, I meant that it is no problem at all to keep it. We can postpone the fsf-assignment thing was what I was referring to. I'll add in the compilation-mode support - keeping references to original authors.

[wasamasa]

I dimly remember having written the new imenu code and have a FSF copyright assignment. I don't see any harm including that basic version (compared to the code it used to be before I've replaced it).

edit: I've remembered wrong: #121 (comment)

[theothornhill]

I think I'd rather not keep code only for the sake of keeping it. I think I'll remove it, and if enough people complain on here I'll bring it back in.

[josteink]

edit: I've remembered wrong: #121 (comment)

That link still brings up the issue of lexical scope.

With a new, simpler/smaller code base it actually sounds doable.

Should we aim for that?

[theothornhill]

That link still brings up the issue of lexical scope.

It was lexical until yesterday.

The small patch I mentioned gave warnings when not dynamic scope. I want to remove it, though, but lambda indentation does not work without it. It is only one function keeping us from lexical scope.

[theothornhill]

Correction: It is lexical again now. Added to top comment as well.

[wasamasa]

Alright, if both of you prefer to do away with it, wouldn't it make sense then to remove the imenu tests? Most build failures are their fault.

[josteink]

Alright, if both of you prefer to do away with it, wouldn't it make sense then to remove the imenu tests? Most build failures are their fault.

Absolutely. No need to have tests for stuff we don't currently deliver.

I just don't think the CI has been a priority at all until now :)

[theothornhill]

Compilation support added and no, I haven't looked at CI yet :)

[josteink]

I guess CI will get there in due time, if you make a PR for this to replace master 😃

[theothornhill]

Yeah! Most tests are passing now, however, there are some edge cases in indentation-tests that I need to work on 👍

[josteink]

Oh yeah. Only test failing now is the compiler directives thing, which was kinda annoying/tricky to get right.

That said:

1. I don't consider them crucial right now
2. And looking at the test-case using visual inspection, they do look right, so I suspect it's a test-issue, more than a test showing an actual defect.

Considering the amount of issues we've had trying to get fontification tested properly, I wouldn't find that surprising in the least.

[jesse-black]

I can do the copyright assignment if someone can point me where to start

[josteink]

It's a bit involved, but nothing impossible: See this link for details.

[binki]

I will try to start working on the copyright assignment.

[theothornhill]

@josteink
Things are really coming along now.
Can you try a couple of things (when you have time)

I've removed the monkey patches from the code, so there is only csharp mode related code here. However, that means that you have to patch emacs instead (...sorry)

Steps to test now:

1. compile emacs with below patch ( or eval this after loading csharp-mode: paste )
2. run indentation tests
3. notice that all but one case works now. (dammit)

yield return new InnerA {
    PropA = 1,
    PropB = 2
};

yield return new InnerA.InnerB { // This one is incorrect.
    PropA = 1,
        PropB = 2
        };

What happens here is actually that the InnerA.InnerB is what's causing this. If I remove the .InnerB everything works again. What do you think? I know you have been thinking about that part for a while (#95). I really cannot find where this is happening, but maybe you understand c-looking-at-or-maybe-in-bracelist from cc-engine.el. I sure don't...

Ok, whats next?

Since it seems we are getting paperwork from @jesse-black and @binki, and that takes a little time, I think we should try to get that patch added to emacs 28. If that happens we have two options:

1. Merge this to master and backporting monkey patch again for emacs version < 28
2. Keep old csharp-mode in melpa, only marking this repo as deprecated for emacs 28, and then people can stop downloading the old one at some point in time.

What do you think?

diff --git a/lisp/progmodes/cc-engine.el b/lisp/progmodes/cc-engine.el
index 4e336c0a06..b415e4b821 100644
--- a/lisp/progmodes/cc-engine.el
+++ b/lisp/progmodes/cc-engine.el
@@ -12011,7 +12011,8 @@ c-looking-at-inexpr-block
 			 (> (point) closest-lim))
 		  (not (bobp))
 		  (progn (backward-char)
-			 (looking-at "[]).]\\|\\w\\|\\s_"))
+			 (or (looking-at "[\]\).]\\|\w\\|\\s_")
+                             (looking-at ">")))
 		  (c-safe (forward-char)
 			  (goto-char (scan-sexps (point) -1))))
 
@@ -12085,7 +12086,11 @@ c-looking-at-inexpr-block
 			  (setq passed-bracket-pairs 1)
 			  (setq bracket-pos (point))))
 		      'maybe)
-		  'maybe))))
+		  'maybe)
+		(if (or (looking-at "([[:alnum:][:space:]_,]*)[ \t\n]*=>[ \t\n]*{")
+			(looking-at "[[:alnum:]_]+[ \t\n]*=>[ \t\n]*{"))
+		    ;; If we are at a C# lambda header
+		    (cons 'inexpr (point))))))
 
       (if (eq res 'maybe)
 	  (cond

[josteink]

Hey again.

Haven't had time to look into any of your new changes in detail yet, but I found this super-strange case, and I can reproduce it consistently:

First field in a class is always fontified incorrectly:

Feel free to take a look at that, while I check your stuff out 😄

[theothornhill]

Feel free to take a look at that, while I check your stuff out 😄

Thanks! Found it and fixed it in 403af83.

In the end I should really go through every c-lang-defconst. I feel there are some bugs, such as this one lurking around.

[josteink]

Thanks! Found it and fixed it in 403af83.

Amazing! Confirmed works.

Things are really coming along now.

This is approaching production-quality as far as I'm concerned.

I've removed the monkey patches from the code, so there is only csharp mode related code here. However, that means that you have to patch emacs instead (...sorry)

No need to be sorry. Getting rid of monkey-patching has been a long-standing goal.

compile emacs with below patch ( or eval this after loading csharp-mode: paste )

Yeah that works exactly like you said.

notice that all but one case works now. (dammit)

I'll be honest and say that's an edge-case. It's not crucial for me to get this fixed before merging to master.

What happens here is actually that the InnerA.InnerB is what's causing this. If I remove the .InnerB everything works again. What do you think?

I haven't worked with this in any detail for a good time now. I really can't tell either.

Ok, whats next? ... Since it seems we are getting paperwork from @jesse-black and @binki, and that takes a little time, I think we should try to get that patch added to emacs 28.

Definitely agreed. And I see you are already proposing that. You have my full support!

If that happens we have two options:

It's Emacs. It's GNU. IMO the end goal should be to get csharp-mode mainlined, but that's going to take time.

Until we have it mainlined I think the best option is the first you propose:

Merge this to master and backporting monkey patch again for emacs version < 28

If we get this mainlined, there will still probably be a need for csharp-mode for older Emacs-versions, but we can mark this repository as deprecated. And when we get that far, we can decide what the best way to obsolete this repo is.

But that's still a bit far off. Let's take one thing at a time?