-
Notifications
You must be signed in to change notification settings - Fork 261
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
feat: Plugin support #1739
feat: Plugin support #1739
Changes from 52 commits
7abd760
daa54ee
867015b
54218fc
8951357
90d8255
39eca00
4b8db03
3d9553c
3664526
4539424
27bd135
2b87a90
98b6d97
93a170c
7a71322
31578e8
3b82af7
ed40974
d40790b
b3d9683
2516be6
0b104e4
b99acbd
1c8084c
9b405de
62ad0ed
52821b3
647b233
7698e7c
eeb264b
d5b653f
6e236bd
9f9e3be
e8ddf7c
0f3ee09
a27836e
4e38b80
40b6a35
b09f5d5
e4c58e5
6becf88
57f7589
4e52ba6
f9c3a17
1d34931
8f8a135
3a4d7ab
749c3f0
6151452
c42d10c
15855d3
b6fcbd8
1fd20d4
6bdcd98
7a5c48b
e17738b
64fc6a2
21188aa
568f1a5
2463c10
eb35472
223c9f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Microsoft.Dafny.Plugins; | ||
|
||
namespace Microsoft.Dafny; | ||
|
||
/// <summary> | ||
/// This class wraps an assembly and an extracted configuration from this assembly, | ||
/// The configuration provides the methods to parse command-line arguments and obtain Rewriters | ||
/// </summary> | ||
public class Plugin { | ||
public PluginConfiguration PluginConfiguration; | ||
|
||
private readonly string pluginPath; | ||
private readonly Assembly assembly; | ||
private System.Type[] rewriterTypes; | ||
|
||
private Plugin(string path, string[] args) { | ||
pluginPath = path; | ||
assembly = Assembly.LoadFrom(pluginPath); | ||
rewriterTypes = CheckPluginForRewriters(assembly); | ||
PluginConfiguration = LoadConfiguration(assembly, args, rewriterTypes); | ||
} | ||
|
||
public static Plugin Load(string pluginPath, string[] args) { | ||
return new Plugin(pluginPath, args); | ||
} | ||
|
||
class AutomaticPluginConfiguration : PluginConfiguration { | ||
private Func<ErrorReporter, Rewriter>[] rewriters; | ||
public AutomaticPluginConfiguration(Func<ErrorReporter, Rewriter>[] rewriters) { | ||
this.rewriters = rewriters; | ||
} | ||
|
||
public override Rewriter[] GetRewriters(ErrorReporter errorReporter) { | ||
return rewriters.Select(funcErrorReporterRewriter => | ||
funcErrorReporterRewriter(errorReporter)).ToArray(); | ||
} | ||
} | ||
|
||
public static IEnumerable<System.Type> GetConfigurationsTypes(Assembly assembly) { | ||
return assembly.GetTypes() | ||
.Where(t => t.IsAssignableTo(typeof(PluginConfiguration))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the sake of consistency within the same class (single letter identifiers), I'd use |
||
} | ||
|
||
private static System.Type[] CheckPluginForRewriters(Assembly assembly) { | ||
System.Type[] rewriterTpes = assembly.GetTypes().Where(t => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the sake of consistency within the same class (single letter identifiers), I'd use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it necessary to specify |
||
t.IsAssignableTo(typeof(Rewriter))).ToArray(); | ||
// Checks about the plugin to be well-behaved. | ||
if (!rewriterTpes.Any()) { | ||
throw new Exception($"Plugin {assembly.Location} does not contain any Microsoft.Dafny.Plugins.Rewriter"); | ||
} | ||
|
||
return rewriterTpes; | ||
} | ||
|
||
private static PluginConfiguration LoadConfiguration(Assembly assembly, string[] args, System.Type[] rewriterTypes) { | ||
PluginConfiguration pluginConfiguration = null; | ||
foreach (var configurationType in GetConfigurationsTypes(assembly)) { | ||
if (pluginConfiguration != null) { | ||
throw new Exception( | ||
$"Only one class should extend Microsoft.Dafny.Plugins.PluginConfiguration from {assembly.Location}. Please remove {configurationType}."); | ||
} | ||
|
||
pluginConfiguration = (PluginConfiguration)Activator.CreateInstance(configurationType); | ||
|
||
if (pluginConfiguration == null) { | ||
throw new Exception($"Could not instantiate a {configurationType} from {assembly.Location}"); | ||
} | ||
|
||
pluginConfiguration.ParseArguments(args); | ||
} | ||
|
||
pluginConfiguration ??= new AutomaticPluginConfiguration( | ||
rewriterTypes.Select<System.Type, Func<ErrorReporter, Rewriter>>((System.Type rewriterType) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the type arguments to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to remove it several times (because the IDE suggests it to me), but every time I do, dotnet compiler says it needs that type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The explicitly typed lambda expressions should be redundant due to the type arguments to pluginConfiguration ??= new AutomaticPluginConfiguration {
Rewriters = rewriterTypes.Select(CreateRewriterFactory).ToArray()
};
return pluginConfiguration;
}
private static Func<ErrorReporter, Rewriter> CreateRewriterFactory(System.Type rewriterType) {
return errorReporter => (Rewriter)Activator.CreateInstance(rewriterType, new object[] { errorReporter });
} I leave it up to you if you want to apply this change. |
||
(ErrorReporter errorReporter) => | ||
Activator.CreateInstance(rewriterType, new object[] { errorReporter }) as Rewriter).ToArray()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
return pluginConfiguration; | ||
} | ||
|
||
public IEnumerable<IRewriter> GetRewriters(ErrorReporter reporter) { | ||
return PluginConfiguration.GetRewriters(reporter).Select(rewriter => new PluginRewriter(reporter, rewriter)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
|
||
namespace Microsoft.Dafny.Plugins; | ||
|
||
/// <summary> | ||
/// Plugins should define a class that extends PluginConfiguration, | ||
/// in order to receive plugin-specific command-line arguments by overwriting the method `ParseArguments` | ||
/// It is also used to provide to Dafny a list of Rewriter using the method `GetRewriters` | ||
/// | ||
/// If the plugin defines no PluginConfiguration, then Dafny will instantiate every sub-class | ||
/// of Rewriter from the plugin, providing them with an ErrorReporter in the constructor | ||
/// as the first and only argument. | ||
/// </summary> | ||
public abstract class PluginConfiguration { | ||
/// <summary> | ||
/// A Microsoft.Dafny.Plugins.PluginConfiguration will be automatically instantiated an arguments | ||
/// will be provided to the plugin by the method `ParseArguments``; | ||
/// </summary> | ||
/// <param name="args">The arguments passed to the plugin</param> | ||
public virtual void ParseArguments(string[] args) { | ||
} | ||
|
||
/// <summary> | ||
/// Override this method to provide specific rewriters to Dafny | ||
/// </summary> | ||
/// <returns>a list of Rewriter that are going to be used in the resolution pipeline</returns> | ||
public virtual Rewriter[] GetRewriters(ErrorReporter errorReporter) { | ||
return Array.Empty<Rewriter>(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
using System.Diagnostics.Contracts; | ||
|
||
namespace Microsoft.Dafny.Plugins { | ||
/// <summary> | ||
/// A class that plugins should extend, in order to provide an extra Rewriter to the pipeline. | ||
/// | ||
/// If the plugin defines no PluginConfiguration, then Dafny will instantiate every sub-class | ||
/// of Rewriter from the plugin, providing them with an ErrorReporter in the constructor | ||
/// as the first and only argument. | ||
/// </summary> | ||
public abstract class Rewriter { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the effort in making the contract as clear as possible here. I think this meets the bar for the feature as long as its marked as experimental. I do think we'll need more of a mechanism to break the build if we change the various AST types in a way that break plugins before we fully commit, though, so good to start thinking about that now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For that, we need access relevant plugins that we intend to support, indeed. This will be the case for compilers soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that's not the right model. We need to explicitly decide on exactly what parts of the implementation plugins have access to, and at the very least document it clearly. Even better, we should use mechanisms to make sure we don't expose things we don't intend to keep the same in the future (i.e. Even with all that, there will still be unfortunate cases where users start relying on something you didn't intend them to (I refer you to the infamous XKCD on that subject: https://xkcd.com/1172/ :) ) and changing things without breaking clients will sometimes be tricky. If you make your API "whatever the users we're aware of are using", though, you're making it that much harder to scale your resources and far more likely you're going to break people and/or tying your hands for refactoring internals for the better in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Note I'm not asking for any deeper changes on this PR right now - it will be great to have folks experiment as long as they know the API isn't finalized yet! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally agree on that ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a need for plugins that aim to support custom compiler back-ends to override anything other than the final I feel the use-case of this PR is underspecified. The title reads "This PR creates an extensible plugin structure, which is primarily used to provide Dafny with extra Rewriters", which I think is more of a how than a why. My impression from our conversations is that this PR is meant to allow custom back-ends to add additional error reporting. I would think that this only requires adding errors after resolution has completed entirely. Is that right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Plugin support is not about just compiler back-ends (see last answer).
I added a statement after the how to explain why.
This PR goes beyond our conversation indeed. It opens the door to plugins actually rewriting the AST before it is resolved (see PreResolve()), and with little effort to later support custom back-ends later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New API looks good :) |
||
/// <summary> | ||
/// Used to report errors and warnings, with positional information. | ||
/// </summary> | ||
protected readonly ErrorReporter Reporter; | ||
|
||
/// <summary> | ||
/// Constructor that accepts an ErrorReporter | ||
/// You can obtain an ErrorReporter two following ways: | ||
/// * Extend a PluginConfiguration class, and override the method GetRewriters(), whose first argument is an ErrorReporter | ||
/// * Have no PluginConfiguration class, and an ErrorReporter will be provided to your class's constructor. | ||
/// | ||
/// Then you can use the protected field "reporter" like the following: | ||
/// | ||
/// reporter.Error(MessageSource.Compiler, token, "[Your plugin] Your error message here"); | ||
/// | ||
/// The token is usually obtained on expressions and statements in the field `tok` | ||
/// If you do not have access to them, use moduleDefinition.GetFirstTopLevelToken() | ||
/// </summary> | ||
/// <param name="reporter">The error reporter. Usually outputs automatically to IDE or command-line</param> | ||
public Rewriter(ErrorReporter reporter) { | ||
Contract.Requires(reporter != null); | ||
Reporter = reporter; | ||
} | ||
/// <summary> | ||
/// Override this method to obtain a module definition after the entire resolution pipeline | ||
/// You can then report errors using reporter.Error (see above) | ||
/// </summary> | ||
/// <param name="moduleDefinition">A module definition after it | ||
/// is resolved, type-checked and SCC/Cyclicity/Recursivity have been performed</param> | ||
public virtual void PostResolve(ModuleDefinition moduleDefinition) { | ||
Contract.Requires(moduleDefinition != null); | ||
} | ||
|
||
/// <summary> | ||
/// Override this method to obtain the final program after the entire resolution pipeline | ||
/// after the individual PostResolve on every module | ||
/// You can then report errors using reporter.Error (see above) | ||
/// </summary> | ||
/// <param name="program">The entire program after it is fully resolved</param> | ||
public virtual void PostResolve(Program program) { | ||
Contract.Requires(program != null); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I would even do this work in the
Load
method and then pass the assembly to the constructor.Also, consider using a record to remove the need for a constructor, like so:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great alternative. I like to put arguments to the class too. Note that it's
Assembly.LoadFrom
, notAssembly.Load
.