Inconsistency when comparing compiled methods

[chisandrei]

Bug description

When comparing two compiled methods that have the same code but different selectors, they can either be equal of different depending on whether or not they have pragmas.

To Reproduce This shows the issue. We create first two compiled methods with the same code and different selectors. They are not equal.

Object subclass: #FooBar2
	instanceVariableNames: ''
	classVariableNames: ''
	package: 'FooBar'.
	
#FooBar2 asClass compile: 'foo1234
	^ 1'.
#FooBar2 asClass compile: 'bar1234
	^ 1'.

(#FooBar2 asClass >> #foo1234) 
	sameLiteralsAs: (#FooBar2 asClass >> #bar1234) "false"

But now we just add the same pragma to the two methods and they become equal.

Object subclass: #FooBar
	instanceVariableNames: ''
	classVariableNames: ''
	package: 'FooBar'.
	
#FooBar asClass compile: 'foo123
	<haba>
	^ 1'.
#FooBar asClass compile: 'bar123
	<haba>
	^ 1'.
	
(#FooBar asClass >> #foo123) 
	sameLiteralsAs: (#FooBar asClass >> #bar123) "true"

Expected behavior The expected result is for the two compiled methods to be different in both cases. Adding a pragma should not make them equal.

Version information: Tested in Pharo 10, 11 and 12

Expected development cost The fix could be to update AdditionalMethodState>>#analogousCodeTo: to also test explicitly for the selector:

analogousCodeTo: aMethodProperties
	| bs |
	self class == aMethodProperties class ifFalse:
		[^false].
	(bs := self basicSize) = aMethodProperties basicSize ifFalse:
		[^false].
	1 to: bs do:
		[:i|
		((self basicAt: i) analogousCodeTo: (aMethodProperties basicAt: i)) ifFalse:
			[^false]].

       "--------"
       self selector = aMethodProperties selector 
		ifFalse: [ ^ false ].
       "--------"

	^true

If the two compiled methods do not have an additional state, then the selector is used during the comparison. But if the method has AdditionalMethodState then AdditionalMethodState>>#analogousCodeTo: does not check the selector during the comparison.

[hernanmd]

Hi Andrei,

This is weird, because if I add a test for your case it passes:

testSameLiteralsAsBetweenMethodsWithPragmas
   	| method1 method2 |

	method1 := TestCase class compiler
			compile: 'foo1234
			<haba>
	^ 1'.

	method2 := TestCase class compiler
			compile: 'bar1234
			<haba>
	^ 1'.

 	self deny: method1 equals: method2.
	self deny: (method1 sameLiteralsAs: method2)

It seems that evaluation of methods from a #DoIt does not include a Pragma in the AdditionalMethodState.

[chisandrei]

@hernanmd Just tried this in Pharo 12, and indeed there seems to be a difference between compiling methods during tests and compiling methods in the browser and using do its.

Here I compiled a method in the code browser and it does not have the source code in the additional method state:

But methods compiled during tests have the source code. I think the compilation environment might be different during tests.

[hernanmd]

Hi @chisandrei Since probably compiling and comparing methods in a DoIt is not a standard use case, do you suggest to keep looking at this issue or a potential fix?

[MarcusDenker]

I want to look at this as this looks really odd... I add it to my (far too long) things that I will look at next

[chisandrei]

@hernanmd I think it is also not just related to DoIts. For example you can have an action in an application that generates code. The same issue I think will be there. Just seems that it is not that easy to write tests to catch this.

[hernanmd]

Just wanted to share I executed in parallel both modes. Left is from the Playground using the script below, right is from the Browser in a test. For now I ignore if having an unbound AST is related:

You can see the execution from the browser has aPragma which has method nil and lookupClass as nil.

| newClass |
newClass := (ShiftClassInstaller new make: [ :aClassBuilder |
		aClassBuilder
			name: 'FooBar';
			superclass: Object;
			package: 'FooBar' ]).
	
newClass compile: 'foo123
	<haba>
	^ 1'.
newClass compile: 'bar123
	<haba>
	^ 1'.
	
(newClass >> #foo123) sameLiteralsAs: (newClass >> #bar123) 

At some point the issue seemed to be IRMethod>>generate in the pragmas: setup (because from the screenshot somehow compiling from a Playground builds the AST and from a method does not), but I'm close to discarding the problem could be there. Also every evaluation in a Playground involves a RBDoItMethodNode, so I'll keep checking. Marcus told me it could be related with the Pragma cache.

[estebanlm]

@chisandrei can you make a pull request?

[chisandrei]

Added https://github.com/pharo-project/pharo/pull/16302 with a possible fix

[Ducasse]

Tx!