Adding local functions #59
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Function declarations now need to derive from StatementSyntax so that we can use them anywhere we can use statements. MemberSyntax is no longer needed - it was necessary to make functions usable as statements in a special scenario, but now they're always statements. I don't think we need to have separate MethodDeclarationSyntax (which would be a MemberSyntax) and LocalFunctionStatementSyntax like C# does, at least for now, because we don't even have classes and don't need to differentiate these.
… variables aren't evaluated). In order to allow calling functions before their declaration, we have to declare all functions in each scope before binding the scope itself. Previously, binding functions was done in a separate step from binding the root statements, and they were lowered at the same time. Now, binding all functions happens during the binding phase when all other statements are bound and lowering happens later during LowerProgram. This allows a better separation of concerns (binder can be independent from the lowerer and we can get the binding result without lowering). BoundProgram renamed to LoweredProgram to better describe its usage.
Previously, GlobalVariableSymbol needed to be separate from LocalVariableSymbol because there was no way to use a (non-global) variable that was declared outside of the current function. Now that local function variable capturing was added, this *just works* and there is no need for special handling of global variables.
This isn't really necessary. We can use the normal lookup rules for a scope to get the previous submission's variables and functions if we just make it our parent scope. No need to copy and redeclare all symbols.
I'm not sure why this was ever needed.
…unctions for SourceCodeKind.Script
|
Wow. I'm proud of the fact that a PR adding local functions actually reduces the total line count 🎉 😄 |
Neme12
commented
May 21, 2019
| _functionBodies.Add((function, body)); | ||
|
|
||
| // Function declaration is a no-op. | ||
| return new BoundBlockStatement(ImmutableArray<BoundStatement>.Empty); |
There was a problem hiding this comment.
Alternatively, we could create some sort of a no-op bound node.
Neme12
commented
May 21, 2019
| public ImmutableArray<Diagnostic> Diagnostics { get; } | ||
| public ImmutableArray<FunctionSymbol> Functions { get; } | ||
| public ImmutableArray<VariableSymbol> Variables { get; } |
There was a problem hiding this comment.
I don't think this is useful anymore since it's accessible from the scope (and noone actually used these properties anyway after the refactoring I did).
Neme12
commented
May 21, 2019
| { | ||
| public BoundProgram(ImmutableArray<Diagnostic> diagnostics, ImmutableDictionary<FunctionSymbol, BoundBlockStatement> functions, BoundBlockStatement statement) | ||
| public LoweredProgram(ImmutableArray<Diagnostic> diagnostics, ImmutableDictionary<FunctionSymbol, BoundBlockStatement> functions, BoundBlockStatement statement) |
There was a problem hiding this comment.
Let the bikeshedding begin. 😄
Neme12
commented
May 21, 2019
| // } | ||
| // foo() | ||
| // It would require flow analysis to prevent this from compiling. | ||
| throw new Exception("Variable not found."); |
There was a problem hiding this comment.
Sorry, I skipped this part 😄 flow analysis is getting a little too complicated for me, I don't think I can do that. Maybe we could just return a default value (0 or null)?
Neme12
commented
May 22, 2019
| @@ -12,78 +12,51 @@ namespace Minsk.CodeAnalysis.Binding | |||
| internal sealed class Binder | |||
| { | |||
| private readonly DiagnosticBag _diagnostics = new DiagnosticBag(); | |||
| private readonly FunctionSymbol _function; | |||
| private readonly List<(FunctionSymbol function, BoundBlockStatement body)> _functionBodies = new List<(FunctionSymbol function, BoundBlockStatement body)>(); | |||
There was a problem hiding this comment.
I chose a list of tuples instead of a dictionary here because this collection is never actually used for lookup, it's only ever used for enumeration (which a list is more efficient at).
|
Sorry :-( Given the amount of merge conflicts I'm going to close this for now. This issue is now tracked as #147. Feel free to submit a new PR for that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements local functions.
The first 3 commits actually implement local functions and the rest is just refactoring & cleanup. If you don't like the followup changes I did there, feel free to only take the first 3 commits and discard the rest. I recommend reviewing this commit by commit. Each commit has a descriptive message to explain what it does and the reason behind it.
The very last commit adds a compilation option for regular/script mode (inspired by Roslyn) and disallows local functions in regular mode because emitting local functions into IL is a lot harder that just evaluating them so that you don't have to do all the work to emit them later.