Should the compiler warn when a pure routine's return value is not used?

bradcray commented 2 years ago

Summary of Problem

With the caveat that I'm not much of a string programmer, during Advent of Code, I found myself writing cases like:

s.strip();

rather than the required:

s = s.strip();

(which probably stems from my use of chomp() in Perl and request for chomp() in #18849). It took me an embarrassingly long time to realize why my code wasn't working.

This made me wonder whether the Chapel compiler could/should warn the user when a pure routine (one that has no side effects—or maybe we'd want the trigger to be something else) is called and the result is dropped on the floor, because such cases seem more likely to be due to a misunderstanding of the interface rather than intentional.

In module reviews, we've discussed other cases with the potential for confusion, like myArray.reshape(...) and potentially myString.replace(...) which could potentially be misinterpreted. Given the choice of clumsier names that make the lack of modification in-place more apparent or cleaner names and this warning, I'd take the latter.

Steps to Reproduce

Associated Future Test(s): test/studies/adventOfCode/2021/bradc/futures/day14-stripDropReturn.chpl #19028

Configuration Information

Output of chpl --version: chpl version 1.26.0 pre-release (56bd177336)

aconsroe-hpe commented 2 years ago

Bringing in some links to the other two languages I know that support this:

C++ 17 introduced the [[nodicsard]] attribute and C++20 lets you attach a message to it
- A result is marked as used with (void)foo(); or std::ignore = foo(); (see std::ignore)
Rust calls it the #[must_use] attribute and also supports attaching a message
- A result is "marked as used with a let _ = foo();

It is interesting to note that both support this attribute on more than just functions, so you can make a struct that would carry this warning in all uses.

I know user facing pragma/attributes are still not here so perhaps that is not the right trigger for us, but I think this would be useful to have.

I'm curious why you initially have this scoped to pure routines? Do we already do purity analysis? Would it be too chatty to warn when any value isn't used?

bradcray commented 2 years ago

I'm curious why you initially have this scoped to pure routines? Do we already do purity analysis?

We don't (that I can recall, anyway), but I mentioned it because it seems that purity (and other similar properties) have come up frequently over the years in terms of reasoning about parallel safety, optimizability, etc. E.g., we've frequently had conversations like "if we know the function is pure, then we can..." The recent Q&A about "synchronization free" pragmas feels thematically related to me, for example.

Would it be too chatty to warn when any value isn't used?

It would be to me in the general case. Sometimes I'm just sketching out code (as I was for AOC) and want to just do readf() without having to check the return value because it's a toy code and I know the input will be well-formed. But when I call a pure method and drop the result on the floor, it seems like a sure sign that I'm doing something wrong. So my preference would be to have an opt-in (or maybe out) flag that did this for any routine, but to have the pure cases handled separately.

For this specific use-case, I agree that an "I'm a pure function" attribute/tag would be an appropriate way to specify this since it's conveying meta-information to a tool (the compiler) that doesn't really affect the program's execution, just the compiler's behavior w.r.t. warnings. If we had other cases that relied on purity for correctness or optimization purposes, then I'd start to wonder whether we'd want/need to make this more first-class in the syntax to support separate compilation, interoperability with other languages where we can't analyze the source, etc. (of course, the answer to that also depends on where we end up falling on the scale of some annotations being required, how far attributes should go in conveying things outside of the language, what the other uses of purity end up being, etc.)

mppf commented 2 years ago

We could also consider making this an error in non-prototype modules.

lydia-duncan commented 2 years ago

I like the idea of using an attribute for this. I worry that implementing it to avoid false positives and false negatives will be a lot of effort, though

bradcray commented 2 years ago

I worry that implementing it to avoid false positives and false negatives will be a lot of effort, though

Can you say more about that? Are you specifically worried about the cases when a user applies the attribute to a routine incorrectly or fails to apply the attribute when they could/should? Or are you worried that applying the attribute correctly could result in false positives or negatives (which I'm not seeing).

aconsroe-hpe commented 2 years ago

Didn't occur to me earlier, but its interesting the motivating case is about a pure function because I think I've always come across this for effectful functions where the the return value is some error value and the must_use/nodiscard is to lint that callers are doing something with the error. All that said, I think the motivating case with string.strip is also a good one and would make sense to check.

Sidenote that string.strip is not declared a const proc today (a hypothetical purity analysis may just ignore anything not const). And so maybe one step easier than purity would be: "warn when const methods that return a value aren't used". +1 for purity analysis though in general

bradcray commented 2 years ago

because I think I've always come across this for effectful functions where the the return value is some error value and the must_use/nodiscard is to lint that callers are doing something with the error.

Yeah, that case is what would make me want the ability to get those warnings in an opt-in/opt-out basis (where maybe the default is based on whether we're in a prototype module as Michael suggests, but I've admittedly been trying to avoid thinking about that concept much because users reacted so negatively to it). I.e., I know it's not safe to call readf() and ignore the return value for bulletproof code, but when I'm just sketching, maybe I don't care and don't want to be hassled with warnings or errors about it.

And so maybe one step easier than purity would be: "warn when const methods that return a value aren't used".

Maybe, although if the const method were to print something out to the console as its side effect in addition to returning something, that might be acceptable (maybe the return code is extra information which isn't always useful). Such a function wouldn't fulfill the "pure" quality if it were auto-inferred (or at least the one in my head... maybe I'm using the wrong term).

lydia-duncan commented 2 years ago

I worry that implementing it to avoid false positives and false negatives will be a lot of effort, though

Can you say more about that? Are you specifically worried about the cases when a user applies the attribute to a routine incorrectly or fails to apply the attribute when they could/should? Or are you worried that applying the attribute correctly could result in false positives or negatives (which I'm not seeing).

Sorry, that should have been more separated out. I'm worried about false positives and negatives without the attribute to aid. Basically, I'm worried about the compiler trying to determine what should warn without explicit hints from the person that defined the function

chapel-lang / chapel