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

Code Rewriting #599

Closed
wants to merge 12 commits into
base: dev
from

Conversation

Projects
None yet
5 participants
@jjrdk

jjrdk commented Mar 11, 2014

Core, please hold off on merging this

I created the interface ICodeRewriter which rewrites a passed code string. The RewritingFilePreProcessor takes a number of rewriters and uses the cumulatively to rewrite passed code.

Created tests for RewritingFilePreProcessor which are essentially the same tests as for FilePreProcessor, but uses StringRewriter, which is a naive sample rewriter.

No additional dependencies created for projects.

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang Mar 4, 2014

Hey Jacob!

We had a team hangout yesterday and discussed this feature, and we'd really like to get something like this in 😄

Since line processors provide somewhat limited value, we've discussed having a "body processor" for a long time, but since we haven't really needed it, we never merged it, but since you've done some nice work here, we'd like to work with you to get it in 😉 As a first step, we'd like to get the infrastructure into core, that'll pretty much be the interface, ICoreRewriter and get it hooked into the existing FilePreProcessor, the same way ILineProcessor is hooked into the pipeline today.

If you could get that up and running, please send us a PR for further discussion 😁

-Kristian

khellang commented on 7428c4e Mar 4, 2014

Hey Jacob!

We had a team hangout yesterday and discussed this feature, and we'd really like to get something like this in 😄

Since line processors provide somewhat limited value, we've discussed having a "body processor" for a long time, but since we haven't really needed it, we never merged it, but since you've done some nice work here, we'd like to work with you to get it in 😉 As a first step, we'd like to get the infrastructure into core, that'll pretty much be the interface, ICoreRewriter and get it hooked into the existing FilePreProcessor, the same way ILineProcessor is hooked into the pipeline today.

If you could get that up and running, please send us a PR for further discussion 😁

-Kristian

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 12, 2014

Member

thanks for the PR!

some comments:

  • looks like now the infrastructure is incomplete since there is no code responsible for registering any ICodeRewriters. And since they are injected - they should be registered with Autofac, yo ucan have a look at how it's currently done with ILineProcessors
  • the new file preprocessor has a bug as it first inserts the #line directive, and then rewrites the code, which can result in #line being in the wrong place, breaking debugging experience. I think that should be fixeable (though haven't tested it) by moving the call to InsertLineDirective into ProcessFile, after it executes Process method and before returning
  • also one more thing - if the rewriter proceeds to adding additional namespaces, they should be added to the FilePreProcessorResult Namespaces property - which makes me think that maybe the better method to use is to override virtual FilePreProcessorResult Process(Action<FileParserContext> parseAction) and then pass the entire FilePreProcessorResult into the rewriter interface (instead of just string)
Member

filipw commented Mar 12, 2014

thanks for the PR!

some comments:

  • looks like now the infrastructure is incomplete since there is no code responsible for registering any ICodeRewriters. And since they are injected - they should be registered with Autofac, yo ucan have a look at how it's currently done with ILineProcessors
  • the new file preprocessor has a bug as it first inserts the #line directive, and then rewrites the code, which can result in #line being in the wrong place, breaking debugging experience. I think that should be fixeable (though haven't tested it) by moving the call to InsertLineDirective into ProcessFile, after it executes Process method and before returning
  • also one more thing - if the rewriter proceeds to adding additional namespaces, they should be added to the FilePreProcessorResult Namespaces property - which makes me think that maybe the better method to use is to override virtual FilePreProcessorResult Process(Action<FileParserContext> parseAction) and then pass the entire FilePreProcessorResult into the rewriter interface (instead of just string)
@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 12, 2014

Member

Also, looks like the build is broken as one test is failing http://teamcity.codebetter.com/viewLog.html?buildId=111978&buildTypeId=bt949

Member

filipw commented Mar 12, 2014

Also, looks like the build is broken as one test is failing http://teamcity.codebetter.com/viewLog.html?buildId=111978&buildTypeId=bt949

@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Mar 12, 2014

Broken test - embarrassing :-( I ran the tests on my machine, but there seems to be a whitespace comparison issue.

As for how to register rewriters, I'd appreciate some input from the core team. Do you want dynamic rewriters registered with MEF, or static which are registered at load? Static is less flexible, but dynamic means that behavior changes during runtime. I don't know if this will make the user expect already parsed code to be rewritten, which would be tricky.

For the other points: Certainly valid.

jjrdk commented Mar 12, 2014

Broken test - embarrassing :-( I ran the tests on my machine, but there seems to be a whitespace comparison issue.

As for how to register rewriters, I'd appreciate some input from the core team. Do you want dynamic rewriters registered with MEF, or static which are registered at load? Static is less flexible, but dynamic means that behavior changes during runtime. I don't know if this will make the user expect already parsed code to be rewritten, which would be tricky.

For the other points: Certainly valid.

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 12, 2014

Member

IMHO static and dynamic registrations are actually not mutually exclusive here.

I would suggest we stick to the convention adopted by other components and make them statically registered against RuntimeServices (with the default in scriptcs.exe being an empty collection).

However, since all components that make up scriptcs can be modified via modules (which use MEF), you could have a module distributed over nuget that a user can install and add your rewriter to the execution pipeline. In face, using the latest version of scriptcs, you can just drop such a module to the local bin folder and if you run with debug mode, it will be automatically discovered with MEF and will modify your scriptcs runtime automagically. Either way you can still provide dynamic discovery and have the rewriters statically registered too.

Member

filipw commented Mar 12, 2014

IMHO static and dynamic registrations are actually not mutually exclusive here.

I would suggest we stick to the convention adopted by other components and make them statically registered against RuntimeServices (with the default in scriptcs.exe being an empty collection).

However, since all components that make up scriptcs can be modified via modules (which use MEF), you could have a module distributed over nuget that a user can install and add your rewriter to the execution pipeline. In face, using the latest version of scriptcs, you can just drop such a module to the local bin folder and if you run with debug mode, it will be automatically discovered with MEF and will modify your scriptcs runtime automagically. Either way you can still provide dynamic discovery and have the rewriters statically registered too.

@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Mar 13, 2014

I'm not sure if I'm missing something here. I tried to go with the module suggestion and I am able to create a module and get it recognized by the IModuleLoader. However if you look at the LoadModules method of the ScriptServicesBuilder then it passes in a new ModuleConfiguration instance. This instance is created from the Overrides dictionary, but any other property ScriptServicesBuilder itself are not passed. This means that if a module does not register itself in the Overrides dictionary the information is lost.

What is the intended pattern? I couldn't find any documentation around this.

jjrdk commented Mar 13, 2014

I'm not sure if I'm missing something here. I tried to go with the module suggestion and I am able to create a module and get it recognized by the IModuleLoader. However if you look at the LoadModules method of the ScriptServicesBuilder then it passes in a new ModuleConfiguration instance. This instance is created from the Overrides dictionary, but any other property ScriptServicesBuilder itself are not passed. This means that if a module does not register itself in the Overrides dictionary the information is lost.

What is the intended pattern? I couldn't find any documentation around this.

@filipw

This comment has been minimized.

Show comment
Hide comment
@filipw

filipw Mar 13, 2014

Member

You can look at LineProcessors as a reference.
it should be as simple as adding a new method on ServiceOverrides

    public TConfig CodeRewriter<T>() where T : ICodeRewriter
    {
        CodeRewriters.Add(typeof(T));
        return _this;
    }

and a List to contain them. Then, adding, in the RuntimeServices, relevant code to pick them up and register against Autofac.

Then in you module you simply say:

public class MyModule : IModule
{
    public void Initialize(IModuleConfiguration config) {
         config.CodeRewriter<MyRewriter>().CodeRewriter<MyAnotherRewriter>();
    }
}
Member

filipw commented Mar 13, 2014

You can look at LineProcessors as a reference.
it should be as simple as adding a new method on ServiceOverrides

    public TConfig CodeRewriter<T>() where T : ICodeRewriter
    {
        CodeRewriters.Add(typeof(T));
        return _this;
    }

and a List to contain them. Then, adding, in the RuntimeServices, relevant code to pick them up and register against Autofac.

Then in you module you simply say:

public class MyModule : IModule
{
    public void Initialize(IModuleConfiguration config) {
         config.CodeRewriter<MyRewriter>().CodeRewriter<MyAnotherRewriter>();
    }
}
@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Mar 13, 2014

That is pretty much exactly the code that I have - jjrdk@ffc0239

But as I said, the IModuleConfiguration instance that gets passed to the module initialization does not seem to be the same as the ScriptServicesBuilder.

jjrdk commented Mar 13, 2014

That is pretty much exactly the code that I have - jjrdk@ffc0239

But as I said, the IModuleConfiguration instance that gets passed to the module initialization does not seem to be the same as the ScriptServicesBuilder.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 27, 2014

Member

@jjrdk one of the unit tests is failing. Can you look into this?

Member

glennblock commented Mar 27, 2014

@jjrdk one of the unit tests is failing. Can you look into this?

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Mar 27, 2014

Member

@jjrdk here is the test detail

RewritingFileProcessorTests+ProcessFileMethod.RewritesScript

 Current failure:       599 #933    Changes (11)    23 Mar 14 09:34
First failure:          599 #920    Changes (6) 11 Mar 14 12:53

Assert.Equal() Failure
Position: First difference is at position 13
Expected: using System;

#line 2 "script1.csx"
Console.WriteLine(@"blah");
Actual: using System;

      #line 2 "script1.csx"
      Console.WriteLine(@"blah");

at ScriptCs.Tests.RewritingFileProcessorTests.ProcessFileMethod.RewritesScript() in c:\BuildAgent\work\8cec70835ab14e6b\test\ScriptCs.Engine.Roslyn.Tests\RewritingFileProcessorTests.cs:line 96
« Hide stacktrace

Member

glennblock commented Mar 27, 2014

@jjrdk here is the test detail

RewritingFileProcessorTests+ProcessFileMethod.RewritesScript

 Current failure:       599 #933    Changes (11)    23 Mar 14 09:34
First failure:          599 #920    Changes (6) 11 Mar 14 12:53

Assert.Equal() Failure
Position: First difference is at position 13
Expected: using System;

#line 2 "script1.csx"
Console.WriteLine(@"blah");
Actual: using System;

      #line 2 "script1.csx"
      Console.WriteLine(@"blah");

at ScriptCs.Tests.RewritingFileProcessorTests.ProcessFileMethod.RewritesScript() in c:\BuildAgent\work\8cec70835ab14e6b\test\ScriptCs.Engine.Roslyn.Tests\RewritingFileProcessorTests.cs:line 96
« Hide stacktrace

@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Mar 27, 2014

Dealt with the failing test. Passed locally (yes I know bad excuse), but somehow some extra whitespace was being added on the build server.

jjrdk commented Mar 27, 2014

Dealt with the failing test. Passed locally (yes I know bad excuse), but somehow some extra whitespace was being added on the build server.

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang May 30, 2014

Member

How is this coming along? Who's on the ball here? 😄

Member

khellang commented May 30, 2014

How is this coming along? Who's on the ball here? 😄

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph May 30, 2014

Contributor

@jjrdk I guess you can probably handle it yourself but if you like we can meet up one evening in Zurich and pair on it to get it rebased etc.

Contributor

adamralph commented May 30, 2014

@jjrdk I guess you can probably handle it yourself but if you like we can meet up one evening in Zurich and pair on it to get it rebased etc.

@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Jun 1, 2014

I still have the question about the IModuleConfiguration reference from above. I made the necessary changes in my branch, but wanted to get someone to validate that I haven't misunderstood something.

If you have time to have meet and get it cleaned up, then I would definitely appreciate it.

jjrdk commented Jun 1, 2014

I still have the question about the IModuleConfiguration reference from above. I made the necessary changes in my branch, but wanted to get someone to validate that I haven't misunderstood something.

If you have time to have meet and get it cleaned up, then I would definitely appreciate it.

@@ -2,6 +2,6 @@
{
public interface IModule
{
void Initialize(IModuleConfiguration config);
void Initialize(IServiceOverrides<IModuleConfiguration> config);

This comment has been minimized.

@glennblock

glennblock Jun 2, 2014

Member

@jjrdk, this will break all existing implementers of modules. Why are you changing the signature?

@glennblock

glennblock Jun 2, 2014

Member

@jjrdk, this will break all existing implementers of modules. Why are you changing the signature?

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Jun 2, 2014

Member

Hi @jjrdk, I took a quick look and left you some feedback.

Member

glennblock commented Jun 2, 2014

Hi @jjrdk, I took a quick look and left you some feedback.

@@ -16,6 +16,6 @@ public interface IScriptServicesBuilder : IServiceOverrides<IScriptServicesBuild
IScriptServicesBuilder LogLevel(LogLevel level);
IScriptServicesBuilder LoadModules(string extension, params string[] moduleNames);
IModuleConfiguration LoadModules(string extension, params string[] moduleNames);

This comment has been minimized.

@glennblock

glennblock Jun 2, 2014

Member

Why does this change to return IModuleConfiguration? This breaks the fluent API.

@glennblock

glennblock Jun 2, 2014

Member

Why does this change to return IModuleConfiguration? This breaks the fluent API.

@adamralph adamralph added the hold label Jun 15, 2014

@adamralph adamralph added the feature label Jun 24, 2014

@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Jul 19, 2014

I'm going to close this, as it is incompatible with the changes made to support Linux and OSX.

jjrdk commented Jul 19, 2014

I'm going to close this, as it is incompatible with the changes made to support Linux and OSX.

@jjrdk jjrdk closed this Jul 19, 2014

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Jul 19, 2014

Member

@jrdk sorry this hasn't progressed. How about we reopen it and we put some cycles into getting it in post 0.10 which is about to ship? We really like the idea. I am sure we can figure something out.

Member

glennblock commented Jul 19, 2014

@jrdk sorry this hasn't progressed. How about we reopen it and we put some cycles into getting it in post 0.10 which is about to ship? We really like the idea. I am sure we can figure something out.

@glennblock glennblock reopened this Jul 19, 2014

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Jul 19, 2014

Member

@jjrdk we've been really heads down trying to just get what we have stabilized enough to ship as it has been so long and there were many changes. We'll have more time once 0.10 is out the door...

Member

glennblock commented Jul 19, 2014

@jjrdk we've been really heads down trying to just get what we have stabilized enough to ship as it has been so long and there were many changes. We'll have more time once 0.10 is out the door...

@jjrdk

This comment has been minimized.

Show comment
Hide comment
@jjrdk

jjrdk Jul 19, 2014

I appreciate that you re-opened this. I've tried getting the original PR merged to the new structure and it's not that straightforward. Might it not be better to close this and start a fresh PR?

The pattern I have suggested is reasonably simple but needs to play well with the base. Since the base has shifted significantly, I think it might be easier to restart and add the rewriters.

jjrdk commented Jul 19, 2014

I appreciate that you re-opened this. I've tried getting the original PR merged to the new structure and it's not that straightforward. Might it not be better to close this and start a fresh PR?

The pattern I have suggested is reasonably simple but needs to play well with the base. Since the base has shifted significantly, I think it might be easier to restart and add the rewriters.

@glennblock

This comment has been minimized.

Show comment
Hide comment
@glennblock

glennblock Jul 20, 2014

Member

Ok sounds good.

On Friday, July 18, 2014, Jacob Reimers notifications@github.com wrote:

I appreciate that you re-opened this. I've tried getting the original PR
merged to the new structure and it's not that straightforward. Might it not
be better to close this and start a fresh PR?

The pattern I have suggested is reasonably simple but needs to play well
with the base. Since the base has shifted significantly, I think it might
be easier to restart and add the rewriters.


Reply to this email directly or view it on GitHub
#599 (comment).

Member

glennblock commented Jul 20, 2014

Ok sounds good.

On Friday, July 18, 2014, Jacob Reimers notifications@github.com wrote:

I appreciate that you re-opened this. I've tried getting the original PR
merged to the new structure and it's not that straightforward. Might it not
be better to close this and start a fresh PR?

The pattern I have suggested is reasonably simple but needs to play well
with the base. Since the base has shifted significantly, I think it might
be easier to restart and add the rewriters.


Reply to this email directly or view it on GitHub
#599 (comment).

@glennblock glennblock closed this Jul 20, 2014

@adamralph adamralph added wontfix and removed feature hold labels Nov 22, 2014

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