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

Faulty code: don't permit undeclared by default #13286

Merged

Conversation

privat
Copy link
Contributor

@privat privat commented Apr 5, 2023

This PR defaults permitUndeclared to false, meaning that unless instructed otherwise with the setter permitUndeclared:, OpalCompiler will consider the (static) use of undeclared variables to be errors.
Moreover, OCUndeclaredVariableNotice is officially promoted to RBErrorNotice. Until now, it was in some limbo state, neither an error nor a warning. This change means that an AST node with such a notice will answer true to isFaulty.

In order for this change to be successful, the following clients are adapted:

  • Traits: TaAbstractComposition sets permitUndeclared
  • Monticello: MethodAddition sets permitUndeclared
  • All clients of ClassDescription>>compile:*, that includes CodeImport and some tests in newtools, permitUndeclared is set according to a heuristic (is a requestor present) that is comparible with the current usage.
    I think that's the right move since I consider ClassDescription>>compile:* a legacy API, so maintaining its behavior make sense.

Tests are also added. RBCodeSnippet gains a new ivar isFaultyMinusUndeclared (the name is not nice, but it is not used that much) that correspond to being faulty, while permitUndeclared is set to true.
It is used for the Monticello test on snippets and for a new test explicitly made to check permitUndeclared:

There are many small changes with specific goals, so there are a lot of commits (oneliner sometime).
The global diff is less big than I expected.

@jecisc
Copy link
Member

jecisc commented Apr 5, 2023

Hi,
Can you elaborate on ClassDescription>>compile:* been legacy? Do you have plans to use something else in the future?

I have the impression that this is the entry point of most method compilation happening in the image no?

@privat
Copy link
Contributor Author

privat commented Apr 5, 2023

Can you elaborate on ClassDescription>>compile:* been legacy?

  • Receiver (the class) is dubious
  • Name compile is bad, because it also installs
  • Returns a symbol. cf compile: .... methods should return CompiledMethod object #12880
  • Not configurable with optimization options or faulty-modes
  • Most of them ask you for a requestor, but requestors are bad
  • Hard to extend: adding a new parameter double the number of selectors

For me, ClassDescription>>compiler is the way forward.

@privat privat force-pushed the faulty-code-permit-undeclared2 branch from 24baf68 to 5ab9b2d Compare April 5, 2023 11:57
@privat
Copy link
Contributor Author

privat commented Apr 5, 2023

Bootstrap passed 🎉, but test failed 😭 because I fumbled a rebase trying to restore some lost comments (cf #13246). So let's try again

@privat
Copy link
Contributor Author

privat commented Apr 5, 2023

Tests are ok, Windows is still running, but who cares?

@MarcusDenker MarcusDenker merged commit 6fc7bc5 into pharo-project:Pharo12 Apr 6, 2023
1 of 2 checks passed
@privat privat mentioned this pull request Apr 7, 2023
6 tasks
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.

None yet

3 participants