checkDuplicates return value

[fosler]

Currently, the Env classes defined in files envVal and envRef have a void checkDuplicates method that can be used to throw an exception if a list of VAR tokens has any duplicates. This is used by the parser in LetDecls, Formals, Fields, Methods, and Statics constructor :init hooks to catch duplicate identifiers.

I propose to modify this method to return the Set<String> instance constructed in the method body instead of returning nothing (void). This will not break existing code (where the return value is simply ignored), but it can be useful in the OBJ language to check if the sets of Static, Field, and Method identifiers contain any of the 'reserved' identifiers self, super, etc. In the current OBJ version, these identifier sets are checked only for duplicates.

[jashelio]

This sounds OK. I believe the checkDuplicates method currently throws an exception if there is a duplicate and returns silently if not. Of course a better solution would be to change how the method works. Write a new public method that returns the strings. checkDuplicates could call this and based on what's in the set raise an exception or not. If done in the proposed way, I would like to request that the existing invocations of the method actually capture the return value in a dummy variable so that readers are kept aware of everything that method does. Jim

…

On Oct 6, 2021, at 12:43, fosler ***@***.***> wrote: Currently, the Env classes defined in files envVal and envRef has a void checkDuplicates method that can be used to throw an exception if a list of VAR tokens has any duplicates. This is used in let, proc, and class expressions to catch duplicate identifiers. I propose to modify this method to return the Set<String> constructed in the method body instead of being void. This will not break existing code (where the return value is simply ignored), but it is can be useful in the OBJ language to check if the set of static, field, or method identifiers contains any of the 'reserved' identifiers self, super, etc. In the current OBJ version, these identifiers are checked only for duplicates. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEH53GQDFGXACLGD62OR52LUFR4BRANCNFSM5FPDOBDQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[StoneyJackson]

I don't see envVal and envRef in ourplcc/course or ourplcc/plcc. I found checkDuplicates in ourplcc/course/code/TYPE1/tenv. It matches your description, but it's as static method of Type. I'll assume this is the method we're discussing. Adding the new return type to its signature I get:

public static Set<String> checkDuplicates(List<String> varList)

Just looking at this, I would assume the returned set should be the duplicates that this method found. But then I think a better name would be getDuplicates. But based on my understanding of what it does -- throws an exception if a duplicate is found -- I think a void return type is more appropriate. It signals that this method will have side effects, and I consider an exception a side effect.

How about adding a void checkReservedNames() that will raise an exception if it finds an identifier with the same name as a reserved word?

[fosler]

(Sorry for the duplicate response.) Stoney, There is a bit of disconnect between the 'course' repository and what I have on our RIT servers. It's possible that the stuff in 'tenv' you mention is probably the same, but this is what I am currently working with: public static void checkDuplicates(List<Token> varList, String msg) { // throws an exeption if the varList has duplicate vars Set<String> varSet = new HashSet<String>(2*varList.size()); for (Token var: varList) { String str = var.toString(); if (varSet.contains(str)) { msg = "duplicate ID " + str + msg; throw new PLCCException("Semantic error", msg); } varSet.add(str); } } If we change the return type to 'Set<String>', it's a simple matter to return 'varSet'. After making this suggested change, the return value is the set of identifier strings passed in the 'varList' parameter, with no duplicates. (It's *not* the set of duplicate identifiers!) If the method detects a duplicate identifier, it throws an exception, otherwise it returns the set of identifiers. The reason for returning the set of identifiers is that some other client -- in the OBJ language, in particular -- can then interrogate this set to see if there are any "reserved" identifiers in it, and possibly throw an exception if so. The 'checkDuplicates' method has no side effects whatsoever, even given the suggested modification, except for throwing an exception in exactly the same way as before. My latest version in the OBJ language uses a 'checkReserved' static method in the ClassVal class that does the reserved word checking: static String [] reservedIDS = new String [] { "self", "myclass", "superclass", "this", "super" }; public static void checkReserved(Set<String> varSet) { for (String reserved: reservedIDS) if (varSet.contains(reserved)) throw new PLCCException( "Semantic error", "reserved ID: "+reserved ); } The 'varSet' parameter is what would have been returned by 'checkDuplicates' described above. It's worth noting that these methods are called during expression parsing, *not* at runtime. So if parsing is successful -- before evaluation -- one can be assured that there are no duplicate identifiers or spurious uses of reserved identifiers at runtime. I must apologize for plunging ahead with changes to the course files on the RIT servers without incorporating these change into ourPLCC. It stems in part from my desire to make swift pedagogical changes in my RIT offering that I deem important, which is at odds with the consensus-building process that infuses our github repository.

…

On 10/6/21 7:43 PM, Stoney Jackson wrote: I don't see |envVal| and |envRef| in |ourplcc/course| or |ourplcc/plcc|. I found checkDuplicates in |ourplcc/course/code/TYPE1/tenv|. It matches your description, but it's as static method of Type. I'll assume this is the method we're discussing. Adding the new return type to its signature I get: |public static Set<String> checkDuplicates(List<String> varList) | Just looking at this, I would assume the returned set should be the duplicates that this method found. But then I think a better name would be |getDuplicates|. But based on my understanding of what it does -- throws an exception if a duplicate is found -- I think a void return type is more appropriate. It signals that this method will have side effects, and I consider an exception a side effect. How about adding a |void checkReservedNames()| that will raise an exception if it finds an identifier with the same name as a reserved word? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN5MQXCCM7UV42SW5KPZK3UFTNJ5ANCNFSM5FPDOBDQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[jashelio]

Tim, I think my suggestion coincides with Stoney's view that returning a set of identifiers and throwing an exception if there are duplicates, and that some callers are only interested in the exception or lack thereof, makes it seem like the method is serving two related but separate purposes. It is certainly less efficient to create a LIST of identifiers and then check that list for duplicates, but as you said, it's only used at parse time. Again, however, these are very fine points, and as I said before I am OK with using your approach. Jim

[StoneyJackson]

I will not say "no" either. Just offering suggestions/advice/opinions.

Maybe factor out the conversion of Tokens to Strings and Lists to Sets from checkDuplicates?

public static void runStaticChecks(_Start parseTree) {
    List<Token> tokenOccurrences = parseTree.getTokens();
    List<String> idOccurrences = Helpers.toStrings(tokenOccurrences);
    Set<String> ids = Helpers.toSet(idOccurrences);
    checkDuplicates(ids, idOccurrences);
    checkReserved(ids);
}

Edit: clean up format of code.

[StoneyJackson]

Is this issue done?

[StoneyJackson]

Note to self:

1. Move Code from ourPLCC to a new repository named Languages.
2. Transfer this issue to this new repository.

DONE

[StoneyJackson]

@fosler Return void.

[fosler]

All language examples now have checkDuplicates return void. All of the current language implementations ignore the checkDuplicates return value (which had emerged only in a few languages like REF, for example) anyway, so this change does not affect existing code behavior for any of the language examples. For the OBJ language, the implementation of checkDuplicates in 'envRef' also checks for reserved identifiers (such as 'this', 'self', etc.). I have changed the Env class in OBJ/envRef so that it uses a HashSet<String> named 'resSet' to collect reserved identifiers. This allows for a more consistent implementation of checkDuplicates (which checks for reserved identifiers in addition to duplicate identifiers) as well as for the implementation of a new 'checkReserved' method. This new method is used in one place: to check for a reserved identifier when defining a top-level variable binding. An attempt to 'define' a reserved identifier now throws an exception. Only the OBJ/code and OBJ/envRef files are affected by these changes to how reserved identifiers are handled. Only the behavior of top-level variable definitions in language OBJ is affected by this change. Example: In the previous version of OBJ, the following "program" will run without error: define self = 14 In the updated version, this will throw a 'reserved ID' exception.

…

On Fri, 2023-10-13 at 12:07 -0700, Stoney Jackson wrote: @fosler Return void.