Skip to content

Improve faulty compilation - unify handling of undeclared variables #13007

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

Conversation

privat
Copy link
Contributor

@privat privat commented Mar 14, 2023

Note: This is another attempt on #12935.

Previously, running

{
OpalCompiler new evaluate: 'a := 1. a'.
OpalCompiler new evaluate: 'a'.
OpalCompiler new options: #(+optionSkipSemanticWarnings); evaluate: 'a := 2. a'.
OpalCompiler new options: #(+optionSkipSemanticWarnings); evaluate: 'a'.
}

gave #{1 1 nil nil} (and no exception or nothing).

That is not nice.

I have no clue but if I try to guess "why?" such different behavior exists, I suppose it because optionSkipSemanticWarnings might be circumstantially used on frontend-only (parse for highlighting for instance) instead of on full compilation.
With optionSkipSemanticWarnings, undeclared variables are not registered, so do not fill the global Undeclared with garbage. (but maybe that's not the reason and I'm thinking too much)

Nevertheless, this is bad.
In the PR I propose to

  1. unify the behavior (adding optionSkipSemanticWarnings does not change the compiled code)
  2. undeclared variable are only registered if a compiled method is produced (by the backend), and again independently of random options.

Now

{
OpalCompiler new evaluate: 'a := 1. a'.
OpalCompiler new evaluate: 'a'.
OpalCompiler new options: #(+optionSkipSemanticWarnings); evaluate: 'a := 2. a'.
OpalCompiler new options: #(+optionSkipSemanticWarnings); evaluate: 'a'.
}

give #{1 1 2 2 }, it's still discutable but at least it is consistent (and memory efficient).

the PR will also help future and more aggressive PR by reducing the initial behavioral space.

@privat privat changed the title Improve faulty parsing - unify handling of undeclared variables Improve faulty compilation - unify handling of undeclared variables Mar 14, 2023
@privat privat changed the base branch from Pharo12 to Pharo11 March 14, 2023 22:11
@privat privat closed this Mar 15, 2023
@privat privat reopened this Mar 15, 2023
@privat
Copy link
Contributor Author

privat commented Mar 15, 2023

CI logs say #name:superclassName:traitComposition:classTraitComposition:category:instVarNames:classVarNames:poolDictionaryNames:classInstVarNames:type:comment:commentStamp: was sent to nil

yeah, I understand that nil was unprepared to receive such a message.

The question is more, how this PR can achieve that?

@privat
Copy link
Contributor Author

privat commented Mar 15, 2023

I get it, the method called is MCClassDefinition class>>#name:superclassName:traitComposition:classTraitComposition:category:instVarNames:classVarNames:poolDictionaryNames:classInstVarNames:type:comment:commentStamp: (what a nice selector!) except that MCClassDefinition is still nil, therefore something is wrong. :(

@privat privat force-pushed the improve-faulty-parsing-unify-undeclared branch from 62367bf to 4e332ae Compare March 15, 2023 14:12
@privat privat force-pushed the improve-faulty-parsing-unify-undeclared branch from 1ef1d7f to b96efe1 Compare March 15, 2023 20:32
@privat
Copy link
Contributor Author

privat commented Mar 16, 2023

I hope I found the bug. I had to dig in the bootstrap and semi-blindly isolate faulty situations with handwritten .st files and inferring how damn those pesky ! interleaving method definitions with expressions in .st files works.

@privat privat changed the base branch from Pharo11 to Pharo12 March 16, 2023 08:51
@privat
Copy link
Contributor Author

privat commented Mar 16, 2023

Tests are ok minus a unrelated windows test

@MarcusDenker MarcusDenker merged commit 8508b86 into pharo-project:Pharo12 Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants