Skip to content
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

[Compiler] Stateless OpalCompiler #11482

Open
wants to merge 6 commits into
base: Pharo11
Choose a base branch
from

Conversation

dionisiydk
Copy link
Contributor

@dionisiydk dionisiydk commented Jul 27, 2022

  1. PR makes OpalCompiler stateless (with temp state deprecation):

With the recent changes to the compiler it is now clear that we can easily remove #source and #ast variables and reuse the compiler instance for parsing/compilation of multiple sources. This way we can get rid of CompilationContext moving all its state to the compiler. So that we will have logical reference from the method/AST to the producer compiler instance (currently it is the reference to the CompilationContext). Then many tools can be simplified by reusing a same compiler instance for own needs (highlighting for example).

Here PR performs main changes to the users of #compiler:

  • Variables #source and #ast are removed (source is temporally deprecated for integration).
  • API is changed to be #compile:/parse:/#evaluate: with source code as a parameter:
OpalCompiler new noPattern: true; compile: '1+2'.

Versus old style:

OpalCompiler new source: '1+2'; noPattern: true; compile.

The new style API was always there but it is now implemented without instance variables #source and #ast.
Most of the changes in PR is such a rewrite from old style to new one.

  • #source:, #compile, #evaluate methods are deprecated (will be removed after NewTools and Spec fixes).
  • All users of compiler are adopted accordingly

The deprecation of #source variable is required to get safe independent adoption of NewTools and Spec. After that the variables will be removed.

The inline of CompilerContext will be done in separate PR.

  1. Environment parameter is moved from CompilationContext into OCSemanticScope as #targetEnvironment which uses the environment of #targetClass by default.

    • it makes the #environment configuration really working. Previously it did nothing. See the changes in #testCompileWithEnvironment.
  2. Introduce compilation option #optionEmbeddAST to remove #compileDoItFromAST method. Now #doIt compilation simply enables #optionEmbeddAST option (just as #optionEmbeddSources option) and a scope indirection is not needed anymore. See #generateMethod.

…API si changed to be #compile:/parse:/#evaluate: with source code as a parameter. #source: method is removed.

- All users of compiler are adopted accordingly
Environment parameter is moved from CompilationContext into OCSemanticScope as #targetEnvironmnet which is by default uses the environment of #targetClass.
- it makes this #environment configuration really working. Previously it did nothing.
failBlock: [^ self decompile ];
class: self methodClass;
parse.
parse: self sourceCode.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such move of #source: argument is the most of the changes here to remove the state from the compiler

^ self classLayout
resolveSlot: aName asSymbol
ifFound: [:var | var]
ifNone: [ self innerBindingOf: aName ]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local lookup is introduced to find any variable defined in the class without the full environment lookup. It is required to allow the override of an #environment of a class during a compilation.

We should really review and unify all current lookup methods: #lookupVar:#lookupLocalVar:/#bindingOf:/#innerBindingOf:.

{ #category : #'options - settings API' }
CompilationContext class >> optionEmbeddAST [
^ self readDefaultOption: #optionEmbeddAST
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is new option #optionEmbeddAST

@@ -488,7 +498,7 @@ CompilationContext >> noPattern: anObject [
anObject ifTrue: [
semanticScope := semanticScope asDoItScope.
"when we compile an expression, embedd sources, as the resulting method will never be installed"
self parseOptions: #(+ #optionEmbeddSources).
self parseOptions: #(+ #optionEmbeddSources + #optionEmbeddAST).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And #optionEmbeddAST is now default option for doIts

^self generate: methodTrailer
newMethod := self generate: methodTrailer.
self compilationContext optionEmbeddAST ifTrue: [
newMethod propertyAt: #ast put: self ].
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is where optionEmbeddAST is used

| environment method return |
environment := SystemDictionary new.
environment at: #MyClass put: Point copy.
(environment at: #MyClass) environment: environment.
environment at: #MyClass2 put: Association.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test demonstrates the environment override. In the current image it does not work.

@dionisiydk dionisiydk changed the title [IGNORE] stateless OpalCompiler. Variables #source and #ast are removed. … [Compiler] Stateless OpalCompiler Aug 1, 2022
@MarcusDenker
Copy link
Member

failing tests not related

@guillep
Copy link
Member

guillep commented Jan 27, 2023

What's the next step here?
I see many merge conflicts.

@dionisiydk
Copy link
Contributor Author

What's the next step here? I see many merge conflicts.

I am waiting @MarcusDenker review for this idea.

Conflicts are resolved

@dionisiydk
Copy link
Contributor Author

conflicts were badly fixed . I will revisit it. But it does not prevent the review

@MarcusDenker
Copy link
Member

The PR #13152 now adds some more state.. (see discussion)

I need to think about this more (stateless compiler, CompilationContext is for sure now holding on to too much state...)

@privat
Copy link
Contributor

privat commented Mar 28, 2023

I added the cleaning of the state in CompilationContext and OpalCompiler to #12883, I will think about that.

@Ducasse
Copy link
Member

Ducasse commented Nov 25, 2023

So what is the status? I have the impression that the issue is not going to be integratable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: NeedsWork/Discussion
Development

Successfully merging this pull request may close these issues.

None yet

5 participants