[compile warnings] unused method's param

[maboiteaspam]

Hi,

In given example, first parameter of Test.dictionnary_files( sourcePath ) is unused, because i did forget to update it after some changes.
Compiler did not complain about it, where he does usually speak a lot and give a lots of good informations.

use System.Security.Cryptography

class Test

    def main
        source_path = "/home/clement/Projects/test/data"
        
        files_table = .dictionnary_files( source_path )
        
        for file_hash, file_entry in files_table
            if file_entry.count > 1 
                f_path = file_entry[0]
                f_count = file_entry.count
                print 'File [f_path] appears [f_count]'
    
    
    
    def dictionnary_files( sourcePath as String ) as Dictionary<of String, List<of String>>
        files = .list_files( "/home/clement/Projects/test/data" )
        files_table = Dictionary<of String, List<of String>>()
        
        for file in files
            hash_file = .make_hash( file )
            
            tomate as List<of String>?
            if files_table.tryGetValue( hash_file, out tomate)
                tomate.add( file )
            else
                tomate_ = [file]
                files_table.add( hash_file, tomate_ )
                
        return files_table
    
    
    
    def list_files( sourcePath as String ) as String[]?
        return Directory.getFiles(sourcePath, "*", SearchOption.AllDirectories)
        
        
        
    def make_hash( file as String ) as String
        hasher = HashAlgorithm.create("SHA1")
        fs = File.openRead( file )
        digest = hasher.computeHash(fs)
        fs.close
        hash = BitConverter.toString(digest)
        return hash

> cobra --version

Code: Select all

2010-10-18 (svn-post-0.8.0)

> mono --version

Code: Select all

Mono JIT compiler version 2.10.2 (tarball Sat Apr 30 10:15:36 UTC 2011)
Copyright (C) 2002-2011 Novell, Inc and Contributors. http://www.mono-project.com
   TLS:           __thread
   SIGSEGV:       altstack
   Notifications: epoll
   Architecture:  x86
   Disabled:      none
   Misc:          debugger softdebug
   LLVM:          supported, not enabled.
   GC:            Included Boehm (with typed GC and Parallel Mark)

> command-line

Code: Select all

  cobra test.cobra

bye

[Charles]

This is the same behavior as C# and gcc (at least for a .c file and even when using -Wall). The reason is that the implementation of a method may have a legitimate reason to ignore a parameter that it doesn't require. The parameter may still be included for various reasons, such as future-proofing the API, or conforming to the base method's signature.

That being said, I can see how such a warning could be useful, provided there was an easy way to turn it off. One idea would be to introduce a virtual keyword "unused" like so:

class A
    def foo(s as String, i as unused int)
        print s

Thoughts?

[maboiteaspam]

Hi,

Thanks for reply! And for the software !

hmmm ok i think i get your understanding and point of view.

Yes your solution sounds good. For newbie's like me, a warning would be thrown.
For more advised developer, this kind of keyword would clarify the code and avoid warnings at compile time.

Alos, it sounds like a more better solution than make use of 'no-warnings' comment (I have seen this keyword in CobraTokenizer, but i don t have time to inspect more for now).
Make use of 'no-warnings' in this situation, would be too generic, and may suppress other warnings related to others defined arguments.

Until now i found out that is has something todo with CobraParser.paramDecl, and KeywordSpecs, to add the keyword.
But i have not been able to target responsible portion code to throw the warning on compile.
I ll try to look for an existing patch similar to this request, and so, maybe i ll be able to provide you a patch.

bye, thanks for your time

[maboiteaspam]

Hi,

I spended sometime to read source code and get better view of the whole system.

I decided to update,
KeywordSpecs.rawSpecs value to add a keyword `unused` as an `arg modifier`
Param class to add a property isDeclaredAsUnused
CobraParser.paramDecl to identify and declare Param as unused or not

This way i should be able to detect the `unused` keyword during parsing operations, and so mark each Param instances as declaredAsUnused or not.

And that's it.

I m now facing two problems,
1/ how detect param as used/or not within body method's statements
2/ when perform param usage detection, and also maybe under which conditions

For the 1/ I can see 4 solutions, im not sure they are feasible and good,
- Create a new method in CobraParser, that use Parser method's to go up and down in the tree.
Traversing the token's list, method would try, to identify each variable usage, match it against an input arguments.
- Or traverse Method instance, instead of token's list, foreach statement, traverse it and so on until i can find any input argument usage.
- Or grep body method against each input argument name. But here i have definitely no ideas, at this time, on how to read the body content method.
- On each statement instantiation, look for usage of an input argument, mark it as used. It sound's like there is a lot of code to do here.

For the 2/ It is not clear for me to determine when i should launch the verify call.
I saw that CobraParser._parseTokens is calling different declaratives methods.
I m not sure i should perform verification on declaration, or later in a different phase(?) / moment.

Do you know if there is any doc i missed on wiki that may help me to figure out how-to-do ?

Thanks for helps,
bye

[Charles]

I don't think there is an existing doc that will help you. We don't have the compiler internals extensively documented. We rely on docstrings in the code, contracts, assertions and of course, the code itself.

I have good news though. If you search the code for "isUsed" you should see that the compiler is already tracking this info because it needs to in order to warn about local vars.

Also I don't think the compiler goes out of its way to avoid tracking this for parameters. So the information is already there.

And finally, there is a method .checkForUnusedVariables in Members.cobra which as already checking the locals. You just need to check the params and including your new property on them.

So you are much closer that you realize.

Enhancing that method will also help demonstrate to you that the compiler does most of its work after the parsing phase.

If you want to see all the phases, try this:

Code: Select all

cobra -v hello.cobra


One more thing: We have a lot of keywords already, so I may tweak your patch to make the word "unused" into what the C# compiler guys call a "virtual keyword", meaning one that it is only considered a keyword in certain circumstances. Basically, this would be done by removing "unused" from KeywordSpecs and improving CobraParser.paramDecl to look for "unused" as an ID (identifier) after AS.

If you feel up to that, feel free to have at it. If not, I can do it later.

When you are done, add a ticket:

http://cobra-language.com/trac/cobra/newticket

And attach a patch:

http://cobra-language.com/trac/cobra/wi ... bmitAPatch

Thanks!

[maboiteaspam]

Hi,

thanks for helps ! It is much appreciated as i was not taking the right way !

As expected i had to implement it using keywords, i also kept the word unused for now.
I was interesting in looking on how-create-virtual-keyword, but it is big question for me now.

It is still missing test's and maybe assertion's, but for now it is not evident for me.

hope this helps,
bye

[Charles]

The tests go next door in the Tests/ subdirectory that sits next to Source/. You generally want to find related tests about parameters and add something there.

To run tests, use:

Code: Select all

bin/testify ../Tests/300-blah blah


In CobraWorkspace/Developer/ImplementationNotes.text you will find notes about "testify".

For as small this patch will be, there might not be many assertions and unit tests within the compiler itself.

So the paths of interest are:

CobraWorkspace/Developer/ImplementationNotes.text
CobraWorkspace/Source/
CobraWorkspace/Source/bin/testify
CobraWorkspace/Tests/

[Charles]

If you search for "pseudo" in CobraParser.cobra, you'll find an example. Basically you check the text of an "ID" token at a key point with an if-statement.

[hopscc]

Nothing to do with the patch specifically.

Chuck I'm wondering if this syntax in cobra isn't a strange weirdness wrt similar setup elsewhere in the language.

Everywhere else uses 'as XXX' to specify a type and only a type.
in param processing here there's this weird exception where you can specify a pseudo keyword before the type to designate a modifier
describing some non default (uncommon) attribute of the parameter.

I'm wondering if we would be better off supporting/allowing the same syntax as elsewhere
i.e rather than

method(z as vari String)
# or
method(a as int, x as out String, y as inout String)
# param = varID [as [ out | inout | vari ]  Type]

It would be clearer/more consistant supporting optional 'is <modifier-list>' syntax
especially as with this patch the attributes/modifiers are not all necessarily mutually exclusive.

method(z as String is vari)
# or
method(a as int, x as String is out, y as String is inout)

method(x as String is unused, y as String is unused, inout)

# param = varID [as  type [is out | inout | vari ] ]

??

[Charles]

You mentioned a "strange weirdness wrt similar setup elsewhere in the language" The form of "unused" I showed is consistent wrt to "out", "inout" and "vari". But I guess you meant the group of those compared to other things (member decls and local var decls).

The last example had a comma separated list of argument modifiers:

def method(x as String is unused, y as String is unused, inout)

# but  what if we swap the order of the parameters?
def method(x as String is inout, unused, y as String is unused)

See that ", unused," in the middle? So now the comma is being overloaded for separating arguments as well as attributes and I think that ends up looking worse. And I think that "stuff as vari Object?" reads well, but I could be biased just from it being the status quo.

We can continue to discuss and muse however. For example, when it comes to methods and properties, I have long suspected that it's not valuable to separate attributes out with "has". They could just go in the "is" list:

class A

    def foo(i as int) is protected, Conditional("DEBUG")
        ...

I suppose that could be extended everywhere including types (class, interface, struct, enum).

We still have the "is" ambiguity like so:

class A

    var foo = 5 is protected

As you know from previous discussions, I'm currently uncomfortable simply resolving that with a "tie breaker". I think the real problem is that "is" is overloaded in a very bad way. I'm committed to "is" always being available in expressions as the identity comparison (same pointer or atomic value). We then have operators: is, is not, == and <>. I'm also committed to member declarations always starting with <keyword> <name> as in "var a", "def foo", "get x", "enum Bar", etc.

Given those constraints, is there something else we could do? I haven't double checked all the implications, but one idea would be to simply drop the "is" and the commas:

class A

    var foo = 5 protected

    def bar(i as int) protected Conditional("DEBUG")

    # and then circling back to your original point about parameters:
    def bar(i as int inout unused, j as int)

Although I do admit this new syntax looks strange to me.

Another alternative would be to find another word for "is" at the attribute level, or even a symbol, but it couldn't conflict with expressions. And it would need to be palatable, of course.

Btw if any changes are actually made, we'll support the current syntax for a long time.

Thoughts?