Functions called via decorators falsely reported as unused (W0612)

[fake-name]

Steps to reproduce

1. Define a class like:

async def offer_func(request):
    params = await request.json()
    desc_offer = RTCSessionDescription(
        sdp  = params['sdp'],
        type = params['type'])

    pc = RTCPeerConnection()
    pc_id = 'PeerConnection(%s)' % uuid.uuid4()
    pcs.add(pc)

    def log_info(msg, *args):
        logger.info(pc_id + ' ' + msg, *args)

    log_info('Created for %s', request.remote)

    @pc.on('datachannel')
    def on_datachannel(channel):

        @channel.on('message')
        def on_message(message):
            if isinstance(message, str) and message.startswith('ping'):
                channel.send('pong' + message[4:])

    @pc.on('iceconnectionstatechange')
    async def on_iceconnectionstatechange():
        log_info('ICE connection state is %s', pc.iceConnectionState)
        if pc.iceConnectionState == 'failed':
            await pc.close()
            pcs.discard(pc)

    @pc.on('track')
    def on_track(track):
        log_info('Track %s received', track.kind)

        if track.kind == 'video':
            local_video = VideoTransformTrack(track, transform=params['video_transform'])
            pc.addTrack(local_video)

        @track.on('ended')
        async def on_ended():
            log_info('Track %s ended', track.kind)

    # handle offer
    await pc.setRemoteDescription(desc_offer)

    # send answer
    answer = await pc.createAnswer()
    await pc.setLocalDescription(answer)

    return web.Response(
        content_type='application/json',
        text=json.dumps({
            'sdp'  : pc.localDescription.sdp,
            'type' : pc.localDescription.type
        }))

This is part of the example scripts from https://github.com/aiortc/aiortc/blob/master/examples/server/server.py that I'm extending.

Current behavior

Pylint falsely reports that on_ended, on_datachannel, on_message, on_iceconnectionstatechange and on_track are unused variables.

First, they're functions (nitpick). Additionally, they're all used via decorators.

Expected behavior

I'd expect pylint to realize that functions can be called via decorators.

From reading other issue reports, it seems that pylint doesn't know how to parse decorators. However, I'd expect it to fail to not report, rather then generating spurious warnings.

Maybe until pylint supports decorators properly, never emit W0612 for functions if they're decorated at all.

pylint --version output

C:\Code>pylint --version
pylint 2.3.1
astroid 2.2.5
Python 3.6.8 (tags/v3.6.8:3c6b436a57, Dec 24 2018, 00:16:47) [MSC v.1916 64 bit
(AMD64)]

[PCManticore]

@fake-name I think you are expecting too much from a linter. How is a linter supposed to know that those functions you are defining are used from a decoration? Detecting this case would require either a blank except of all the decorated functions, which leads to false negatives, or introspecting the decorator to "figure" out statically what the decorator is doing with their arguments, which again would most likely lead to false positives and it's a hard task to implement correctly, let alone to work in a fast manner. For this case it's just better to disable the error in place.

[fake-name]

How is a linter supposed to know that those functions you are defining are used from a decoration?

I would expect it to never report unused function for functions that are decorated.

If something is basically impossible to determine correctly, why is it emitting the warning?

[PCManticore]

Because it's better to be safe than sorry, pylint's principle is being over verbose in terms of what it emits, letting the user control what messages are actually problems or not.

[belm0]

@PCManticore please reconsider, because it's becoming a common pattern to decorate functions within a context manager scope to have them active in the scope somehow. E.g. stdlib ExitStack:

from contextlib import ExitStack

with ExitStack() as stack:
    @stack.callback
    def cleanup():
        print('done')

    ...

And Trio async library, here running child tasks in parallel:

async with trio.open_nursery as nursery:
    @nursery.start_soon
    async def foo():
        ...

    @nursery.start_soon
    async def bar():
        ...

The likely future is that everyone disables unused-variable globally. To avoid that it needs to have less false positives. Disabling the warning on decorated functions sounds reasonable.

[fake-name]

An alternative would be to have a way to say "these specific context managers are used, even if they don't look it"

force-used-variable=<context-name> or something?

[belm0]

force-used-variable=<context-name> or something?

It would probably be limited to a text match of the decorator method names of the context manager (which itself is probably some internal class of the API you're using).

A workaround I've found is to prefix names of such local functions with underbar (or any other match of the dummy-variables-rgx config setting).

[fake-name]

It would probably be limited to a text match of the decorator method names of the context manager

That would be completely fine. The idea is you can manually exclude certain decorators on a per-file basis.

which itself is probably some internal class of the API you're using

In my OP, the decorators are the intended public interface for a WebRTC interface (see https://github.com/aiortc/aiortc/blob/master/examples/videostream-cli/cli.py).

Unless you mean internal mangled names, or something?

My idea was basically "Ignore all unused for instances of the literal decorator string pc.on" (or some similar implementation.

[belm0]

My idea was basically "Ignore all unused for instances of the literal decorator string pc.on" (or some similar implementation.

"pc" is a user-chosen identifier (it could be passed in as a function arg, you may need to juggle multiple connections, etc.). Hence I think it would likely devolve to text match on method name.

pc = RTCPeerConnection()

Anyway I suspect the feature may be a tough sell since these are just local functions and you can use the existing escape mechanism for unused names.

[PCManticore]

@belm0 What's wrong with doing a local pylint: disable=unused-variable and be done with it? These APIs look completely unintuitive but now it's on the linter to disable all sorts of edge cases to appease all these library authors abusing the features of a language? I'm sorry but this is definitely a tough sell.

[belm0]

Using an underbar prefix on the local is cleaner than disable comment, I'm fine with the former as a workaround.

These APIs look completely unintuitive but now it's on the linter to disable all sorts of edge cases to appease all these library authors abusing the features of a language?

As mentioned, this pattern is encouraged by Python standard packages, such as the ExitStack API for push and callback. I disagree about it being unintuitive-- in my experience on a team of varied skill levels, it takes only a day to become familiar enough with the pattern to use and recognize it.

[fgimian]

I've just encountered this. In my case, I'm declaring an app and a related route as a pytest fixture like so:

@pytest.fixture
def app():
    api= FastAPI()
    api.add_event_handler("startup", database.create_pool)
    api.add_event_handler("shutdown", database.close_pool)
    api.add_middleware(BaseHTTPMiddleware, dispatch=database.middleware)

    @api.get("/")
    async def root(db: PoolConnectionProxy = Depends(get_db)):
        return await db.fetch("SELECT * FROM data")

    return api

This issue affects the root function name above too.

I think I'll just disable the check for now 😄

[slifty]

FWIW I'm bummed that I lose the benefit of this great check simply because I'm using modern features of modern tooling (e.g. FastAPI path decorators).

I'm forced to either lose the benefit of the linter in preventing dead code, or lose the syntactic sugar of @app.get

It would be really wonderful to be able to define a pattern or other set of patterns for decorators that should be considered "use"