uqbar-project / wollok

Wollok Programming Language
GNU General Public License v3.0
60 stars 16 forks source link

Improve error message when object reference is forgotten #1216

Open lgassman opened 7 years ago

lgassman commented 7 years ago

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:

At else line:

At return line:

fdodino commented 7 years ago

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"

clombardi commented 7 years ago

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?

fdodino commented 7 years ago

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.

clombardi commented 7 years ago

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?

fdodino commented 7 years ago

Great news!

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

clombardi commented 7 years ago

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 ) ;


2. 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"
        )
}

3. 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.
fdodino commented 7 years ago

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

npasserini commented 7 years ago

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 notifications@github.com:

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 .

fdodino commented 7 years ago

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

npasserini commented 7 years ago

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

2017-09-04 11:53 GMT-03:00 Fernando Dodino notifications@github.com:

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 .

web-ciu-programacion commented 7 years ago

(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.

clombardi commented 7 years ago

Did it, branch carlos-try-1216