wollok icon indicating copy to clipboard operation
wollok copied to clipboard

Improve error message when object reference is forgotten

Open lgassman opened this issue 7 years ago • 12 comments

This is a real world example. This code was written by a student at his first class day.

	if (estaEnElRango()){
		vuelaEstaDistancia=(energia/5)+10
	}
	else{
		vuelaEstaDistancia=energia/5
	}
          return vuelaEstaDistancia 

The problem is easly to resolve, the "self" reference was forgotten. But it is not easy for a newby, because he is accustomed to invoking structured programming procedures. I would like to read an error message like "receiver object reference is forgotten, maybe 'self'".

The IDE shows these problems: At if line:

  • Couldn't resolve reference to Referenciable 'estaEnElRango'.
  • no viable alternative at input ')'
  • If-statements without else cannot be considered valid expressions as they cannot always yield a value
  • extraneous input '(' expecting ')

At else line:

  • mismatched input 'else' expecting '}

At return line:

  • missing EOF at 'return'

lgassman avatar Aug 17 '17 20:08 lgassman

Well, I tried to solve this issue, but it is very hard to catch because parser interprets estaEnElRango as a reference, and parentheses are split to another token.

Maybe I can hack Wollok DSL validator in "Couldn't resolve reference to Referenciable"

fdodino avatar Sep 02 '17 00:09 fdodino

Hi, just a bit of conjectural thinking. We possibly (depending of the strength of the XText-generated parser) define an additional option for the definition of WFeatureCall, which would be ReceiverLackingFeatureCall, namely, a FeatureCallID followed by either a parameter list or a closure. Then we could add (again, if allowed by XText logic) a validation that signs every occurrence of a ReceiverLackingFeatureCall as an error. If the ID coincides with a selector that is understood by self, then we could suggest a possibly lacking "self.". Do you think this to be feasible?

clombardi avatar Sep 02 '17 15:09 clombardi

Wow, synchronized minds perhaps, but I was currently working with it:

WMemberFeatureCall returns WExpression:
      =>(({WMemberFeatureCall.memberCallTarget=current})? ("." | nullSafe?="?.")?)
      line 228

But this doesn't generate a valid syntax. I still need to understand better EBNF. Afterwards a validation could report an issue, as you proposed @clombardi . I think this could be feasible.

fdodino avatar Sep 02 '17 15:09 fdodino

Hi Fer, I just made a tiny proof of concept using the following toy grammar ProductPrices: prices+=Price* ifExpression=IfTest; Price: name=ID value=INT; IfTest: 'if' '(' condition=Expression ')' 'then' price=Price; Expression:   properExpression = ProperExpression | malformedExpression = MalformedExpression; ProperExpression: receiver=ID '.' selector=ID '()'; MalformedExpression: selector=ID '()';

The following validation @Check   def checkProperExpression(Expression expr) {     if (expr.malformedExpression !== null) {       error(         'Parece andar faltando un receptor aca',         TicketsPackage.Literals.EXPRESSION__MALFORMED_EXPRESSION,         'FaltaReceptor'       );     }   }

works as expected.

Wrt the definition of Wollok, I guess I would try something like the following WFeatureCall : WSuperInvocation | WMemberFeatureCall | WWrongMemberFeatureCall;

WWrongMemberFeatureCall returns WExpression:   wrongFeature=FeatureCallID     (('('       (memberCallArguments+=WExpression (',' memberCallArguments+=WExpression)*)?     ')') | memberCallArguments+=WClosure )   ;

I did this on WollokDsl.xtext, the grammar checker did not comply anything. In order to have this change accepted, I had to change, in the definition of WWrongMemberFeatureCall, the clause feature=FeatureCallID by wrongFeature=FeatureCallID since XText complains about the duplication of a feature property.

I imagine that the need to give the feature of the wrong feature call a different property name is actuall y handy. We could check simply expr.wrongFeature !== null a true value for this condition would signal a WrongMemberFeatureCall. Furthermore, we obtain the invoked selector as the value of the wrongFeature property, hence we could verify whether is a selector acceptable by the current self.

I generated the language infrastructure after the grammar change I propose. It builds subclasses called WSuperInvocation and WMemberFeatureCall, but not WWrongMemberFeatureCall. Moreover, it adds the wrongFeature property to WExpression class. I wonder if this is the desired behavior in terms of the logic (unknown by me) of Wollok expression handling. We could add a {WWrongMemberFeatureCall} clause at the beginning of the WWrongMemberFeatureCall rule, but in such case, I guess that we should ask explicitly for the concrete class of the handled expression. Whom should I ask about such details?

clombardi avatar Sep 02 '17 17:09 clombardi

Great news!

I'll give a try this monday, but also Javi, or Nico, or Pablo should have a better explanation.

fdodino avatar Sep 02 '17 18:09 fdodino

I already gave a try, unfortunately it didn't work. I did this:

  1. Defined a new rule in the grammar, and add it as an aditional option for WFeatureCall . Namely

WWrongMemberFeatureCall returns WExpression:
  {WWrongMemberFeatureCall} 
  wrongFeature=FeatureCallID 
    (('(' 
      (memberCallArguments+=WExpression (',' memberCallArguments+=WExpression)*)? 
    ')') | memberCallArguments+=WClosure  )
  ;
  1. added this validation in WollokDslValidator
	@Check
	@DefaultSeverity(ERROR)
	def receiverMissing(WWrongMemberFeatureCall wrongFeatureCall) {
			report(
				"Falta el objeto receptor", 
				wrongFeatureCall,
				WWRONG_MEMBER_FEATURE_CALL__WRONG_FEATURE, 
				"RECEIVER_MISSING"
			)
	}

  1. wrote the following test
	@Test
	def void testReceiverMissingFeatureCall() {
		val model = '''
			class Tren {
			    method getCantidadPasajeros() {
			        return 3
			    }
			    method dobleDeCantidadPasajeros() {
			    	return getCantidadPasajeros() * 2
			    }
			}			
		'''.parse
		model.assertError(model.eClass, "RECEIVER_MISSING")
	}

The test failed. I got the same errors reported by Leo in the first message for this issue.

I guess that the cause of the failure is the first => in the grammar rule WMemberFeatureCall:

WMemberFeatureCall returns WExpression:
	WPrimaryExpression
	(
		=>({WMemberFeatureCall.memberCallTarget=current} ("."| nullSafe?="?.")) 
		=>feature=FeatureCallID 
		(('(' 
			(memberCallArguments+=WExpression (',' memberCallArguments+=WExpression)*)? 
		')') | memberCallArguments+=WClosure	)
	)*;

If I remove this => from the defintion of this rule, then when I run the generation of XText artifacts, I get a warning message in the console indicating a possible ambiguity in the defined grammar, and some errors arise in the class org.uqbar.project.wollok.formatting.WollokDslFormatter.

At this point, any help of someone that knows what's happening would be great.

clombardi avatar Sep 03 '17 03:09 clombardi

Well, after struggling 2 hours, I can't figure out a better explanation for Carlos. Maybe @tesonep , @npasserini or @javierfernandes have a clue ...

fdodino avatar Sep 04 '17 13:09 fdodino

I am not sure why is that. Is there a branch I can checkout to test it myself?

2017-09-04 10:57 GMT-03:00 Fernando Dodino [email protected]:

Well, after struggling 2 hours, I can't figure out a better explanation for Carlos. Maybe @tesonep https://github.com/tesonep , @npasserini https://github.com/npasserini or @javierfernandes https://github.com/javierfernandes have a clue ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/1216#issuecomment-326969878, or mute the thread https://github.com/notifications/unsubscribe-auth/AEa1OZ-Ujcp40CrALHn5o4e7CQkRPXPwks5sfAHngaJpZM4O6wPG .

npasserini avatar Sep 04 '17 14:09 npasserini

Any branch is ok... I tested Carlos' solution in "parser messages" branch and got the same problem

fdodino avatar Sep 04 '17 14:09 fdodino

I mean the branch with Carlos' attemps to fix the problem.

2017-09-04 11:53 GMT-03:00 Fernando Dodino [email protected]:

Any branch is ok... I tested Carlos' solution in "parser messages" branch and got the same problem

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/1216#issuecomment-326981195, or mute the thread https://github.com/notifications/unsubscribe-auth/AEa1OZ1Em7Y01aS3_GFlHV78VD1UHVLAks5sfA7TgaJpZM4O6wPG .

npasserini avatar Sep 04 '17 14:09 npasserini

(This is Carlos, just logged to github using a different user, sorry for that).

No, I did not dare to open a new branch in the Wollok rep, I should have asked. In any case, I only changed WollokDsl.xtext and added a method to WollokDslValidator . In WollokDsl.xtext, maybe I did not make enough clear that I added WWrongMemberFeatureCall as an additional option for WFeatureCall , after the two current options . I can make a commit on a new branch tonight at home.

web-ciu-programacion avatar Sep 04 '17 20:09 web-ciu-programacion

Did it, branch carlos-try-1216

clombardi avatar Sep 05 '17 03:09 clombardi