Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@ztone
Copy link
Contributor

@ztone ztone commented Jun 23, 2014

update by @adamralph

As merged, this fixes #713 only.


This PR is a proposed solution to get the Mono engine close to be at par with Roslyn and solve most of the Mono issues.

Why this approach
NRefactory failed to parse correctly if script contained blocks { }
Needed a way to fix #713

What this PR does
So the approach I've taken is to split the script into code segments.

  • The lexer : find a few token needed for parsing and disregard stuff in strings, chars and comments so it doesn't interfere with the parsing.
  • Region parser: Eats all tokens until it find either a semicolon or an opening/closed bracket.

For example Console.WriteLine(“hello”); is a region and if(true) { doStuff(); } is also a region. The lexer and parser don't care about the syntax or semantics, it's just used to do a logical split of the code.

  • NRefactory: Each segment is then passed into the NRefactory parser to check if the segment is a class, method or evaluation. It the segment is a method it is rewritten by NRefactory to a method expression to support loose functions.

Segments are also ordered as we want to evaluate classes first, then method prototypes. After that we evaluate method expressions and lastly the evaluation.

If you look at ScriptCs code this approach does make some sense.

class A { you can write anything here }

void Bar() { also here }

Bar( even inside parentheses );

Here we identify three regions/segments. The Mono.CSharp evaluator will handle all the compiler errors that are inside each segment (and NRefactory for methods).

Other

  • Mono engine will return the correct line number for a compilation errors. However, a few method errors are still a bit strange because of the method rewrite. Needs investigating.
  • Before, code like error. didn't cause any failure (as the evaluator regards this as a partial, because of the dot). Handling partial loose functions is going to be a bit tricker and needs to be in another PR.

Please test drive.

Stone

@ztone ztone changed the title Code segments Mono engine using code segments to better parse scripts Jun 23, 2014
@ztone
Copy link
Contributor Author

ztone commented Jul 13, 2014

TBH, I would have liked this PR to be part of the 0.10 release as IMO the mono engine is still restricted without it. However, this is a big change so it’s also a bit risky. That’s why I haven’t been pushing for it. Unfortunately I created it after the code freeze.

The current engine isn’t really user friendly as NRefactory can screw up the parsing if code contains blocks and give unexpected messages. Also often the line error messages are incorrect and exceptions not passed to be handled by ScriptCs.Core. This will be annoying for users scripting on the engine.

As code speaks loader then words here are some code snippets.

Block parsing failure
This will error in current engine put evaluate correctly with this PR

class A { public string X { get { return "Class A"; } } }
if(true) { var x = 10; } 
class B { public string X { get { return "Class B"; } } }
Console.WriteLine(new A().X);
Console.WriteLine(new B().X);

Issue #713
This will error in current engine put evaluate correctly with this PR

var x = 123;
Action a = () => x++;
Console.WriteLine(x);

REPL multiline
This will not work in the current engine but with this PR
> Console.
* Writeline(“Hi”);

Errors

var a = 1;
var b = 2;
Foo(a,b);
public void Foo(int a, int b) { return a + b; }

Current engine will give error message at line 2,2
(2,2): error CS0127: `System.Action<int,int>': A return keyword must not …

While the new engine will return more correctly
ERROR: (4,34) CS0127 `System.Action<int,int>': A return keyword must not …

Also this PR will build bau scripts … checked both bau and bau-msbuild
https://github.com/bau-build/bau/blob/dev/baufile.csx
https://github.com/bau-build/bau-msbuild/blob/master/baufile.csx

Just my 5 cent for now!
Cheers

@filipw
Copy link
Member

filipw commented Jul 13, 2014

this is awesome.

I would suggest that MonoReportPrinter is submitted as a separate PR - we try to keep PRs as lean as focused as possible, and it would make sense to isolate that one. I was actually working on the printer today too but it's even better that you got there first :)

I would suggest the same for REPL multiline, but this looks like it's too coupled to the new code.

Makes sense to you?

@ztone
Copy link
Contributor Author

ztone commented Jul 13, 2014

@filipw ... yes I know it's a big block! There are many issues interconnected here but I tried to keep he commits self contained. I can't see why I couldn't extract both MonoPrinter and MuliLine REPL into a separate PR . But now I'm off to watch the game :)

@adamralph adamralph removed the hold label Jul 30, 2014
@filipw
Copy link
Member

filipw commented Sep 4, 2014

@ztone hey man, do you mind picking this up like we discussed? It would be good to have 3 separate PRs here:

  • printer
  • multiline
  • lexer

This way it would be easier to digest and error-proof. Thanks again, since this work is extremely valuable.

If you need any help let me know.

@ztone ztone force-pushed the code-segments branch 2 times, most recently from a80d687 to a048e5f Compare September 17, 2014 22:00
@ztone
Copy link
Contributor Author

ztone commented Sep 17, 2014

Hi @filipw

I've changed the PR to just include the lexer, which is the bulk of my former PR anyway.
The goal with this lexer is to get up to par with Rosyln. Error reporting will still be off as I've removed the MonoReporter.

Cheers,
Stone

@adamralph adamralph added the mono label Oct 6, 2014
@glennblock
Copy link
Contributor

@filipw are you looking into this?

@filipw filipw mentioned this pull request Jan 24, 2015
@ztone
Copy link
Contributor Author

ztone commented Jan 25, 2015

@filipw @adamralph I rebased this PR.

Once I took a closer look I remembered that this PR has some needed features like keeping track of line numbers (as scripts are being re-ordered before evaluation) and maintains code format as classes and methods are re-formatted by NRefactory in our current solution. This is needed when we want to add our custom mono reporting class. Also because of this reformatting, implementing multiline support for classes is going to be awkward in our current solution. For example, NRefactory will add a missing curly bracket for classes which will mean that the Mono.CSharp will not think it’s an incomplete submission.

Pros:

Cons:

  • This is a large chuck of code that might contain unforeseen parsing bugs, but is has a great deal of unit-tests.
  • A small performance hit as script needs to be pre-parsed.

@filipw
Copy link
Member

filipw commented Jan 26, 2015

great. IMHO this is good to merge. I know this is a massive changeset, but it was expected to be, since it introduces a lexer. I wouldn't worry about introducing bugs that much. Firstly, it has a bunch of tests like you mentioned. Secondly, I tested it out when it was brought forward originally and it worked like a charm.

I say this should go in immediately, as it's definitely an improvement in our Mono support. unless someone wants to chime in? @scriptcs/core

@khellang
Copy link
Member

:shipit:

adamralph added a commit that referenced this pull request Jan 26, 2015
Mono engine using code segments to better parse scripts
@adamralph adamralph merged commit 483e284 into scriptcs:dev Jan 26, 2015
@adamralph
Copy link
Contributor

Thanks @ztone! This will go out in 0.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expressions capturing local vars do not compile in Mono Mono error reporting

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.