Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linter suggests to replace lambda operation expression with lambda function expression #2039

Open
msoeken opened this issue Nov 19, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@msoeken
Copy link
Member

msoeken commented Nov 19, 2024

Describe the bug

Linter suggests to replace operation lambda expression with function lambda expression, even though this would not work, because the callable signature requires an operation.

To Reproduce

https://microsoft.github.io/qsharp/?code=H4sIAAAAAAAAE22QT0vDQBDF7%252FkUj542ElMKnhoqiFUU%252FIO24qHksE3XZKDZze5OLEXy3WXTYIt0LjPMm9%252BDeVQ3xjEWvElvnJN7n15kER2Xc5KlNp6p6JXINMpJJqPxSVwt2lpYjyne2jXxKk8gi14cNphd40MTx5j2HT8RANzLgoU2jEd%252FVze8F9bHCUbWo5A67NcKKgijOIt6YkdckR7wUF%252FGgUAakzR9UrrkKpjgEpOTo1C3L69LYf2KgpYnCGMeZ383XT91kE2z3Z%252Bwh0fEUtI2OA9EF3WnGTxL0uLfc61XsB6zIZOr%252FEAe40pgQy7ztm7eVUmelRMrm8dxhvEYO2d0OYWvTLvdIIRR0reCxE46Tbo8b%252FYg7MAXxjlV8BQbo%252Fw5vot%252BAVKAZZX0AQAA&profile=unrestricted

Expected behavior

No linter warning should appear.

Additional context

This is similar to #1661

@msoeken msoeken added bug Something isn't working needs triage labels Nov 19, 2024
@msoeken msoeken changed the title Linter suggest to replace lambda operation expression with lambda function expression Linter suggests to replace lambda operation expression with lambda function expression Nov 19, 2024
@Manvi-Agrawal
Copy link
Contributor

Manvi-Agrawal commented Nov 19, 2024

@msoeken I think this bug is interesting edge case which might be tricky to tackle. I think this is arising due to function being promoted to operation because of all able conversation rules on Q#. Syntactically, all callables that are declared functions can be declared as operations, and the program will compile. But not all callables that are declared as operations could be converted to functions. Of course, I am leaving nitty gritty details like what if function is passed as an argument, in that case changing function to operation doesnt lead to a compilable Q# program

The example involves a function invoking DumpOperation function being passed as lambda argument. According to my understanding DumpOperation does not work on real hardware. I understand that it might be a little inconvenient for simulation purposes and might require refactoring. Do you have some other practical example(s) in mind for real hardware?

What if somebody decides to create a callable CustomDumpOperation that is a wrapper over current DumpOperation function. And then passed CustomDumpOperation to a operation accepting operation as argument. Then it seems we shouldn't flag it as warning, am I correct? I wonder this might lead to a snow ball effect, encouraging more operations.

What if the CustomDumpOperation is required to be declared as an operation for only few operation calls. And it works perfectly fine as a function for other cases. Is the suggestion to gravitate towards allowing CustomDumpOperation as a operation. I am afraid this kind of defeats the purpose of this lint rule.

Given the fact that this is a lint rule which can be turned off, I believe the current implementation is good enough for regular usage. Some other potential ways to tackle this might be to offer an improved linting functionality: (I) like inclusion/exclusion rules for file paths for each lint, (ii) having an allow list for a function to be treated as a Operation. So, in this case we could instruct QDK to treat DumpOperation as operation so that we don't get the warning. Thoughts?

PS: sent from mobile, not responsible for autocorrect errors

@swernli
Copy link
Collaborator

swernli commented Dec 3, 2024

All functions are operations. But not all operations are functions.

That's actually not the case, functions and operations are entirely distinct. Functions in Q# must be pure, so should have no side effects and have output that is entirely determined by their input. Operations are impure and thus can have side effects or return different results for the same inputs, where the most notable side-effect is allocation and manipulation of external quantum state referred to by qubits. So that is why an operation can allocate qubits but a function can't and also why an operation can call other operations or functions but a function cannot call operations.

@Manvi-Agrawal
Copy link
Contributor

Functions in Q# must be pure, so should have no side effects and have output that is entirely determined by their input. Operations are impure and thus can have side effects or return different results for the same inputs.

Thanks @swernli for the clarification. To clarify, I meant compilation of Q# programs syntactically. I have updated my earlier comment. I think with time, I developed shortcut that a callable doing classical computation should be a function and one doing quantum computation should be a function.

I am curious what do you think about my earlier comment about NeedlessOperation lint and the edge case discovered by Mathias?

@swernli
Copy link
Collaborator

swernli commented Dec 5, 2024

Regarding the original lint, it's technically correct because the lifted operation created from the lambda does not allocate qubits or call any operations. I think the right fix would be to enlighten the lint to make it ignore lambdas entirely. As a workaround that doesn't require any changes to the compiler, the lambda can just include a call to I which is a no-op gate operation and will keep behavior the same while suppressing the lint: Playground

WithSum(qs, q => { I(q); DumpRegister([q]); });

@Manvi-Agrawal
Copy link
Contributor

Manvi-Agrawal commented Dec 5, 2024

As a workaround that doesn't require any changes to the compiler, the lambda can just include a call to I which is a no-op gate operation and will keep behavior the same while suppressing the lint.

I think I have slightly more preference towards this workaround because I assume such cases to be few. The beauty of this approach is that it works without qubits too. Just declare operation NoOp():Unit {} and call it inside the lambda callable. `WithSum(qs, q => { NoOp(); DumpRegister([q]); });

I think the right fix would be to enlighten the lint to make it ignore lambdas entirely.

Hmm, this gets tricky. I am not sure if Q# HIR distinguishes between the lamda and callables. I would prefer for users to have a consistent experience when declaring callables and lambdas. This discussion gives me new ideas like introduce linter properties:

  • excludeLambdas=false/true: Whether to exlude lambdas for consideration? Default="false"
  • AggressiveChecking: whether to look out for misusage of NoOp and I. Default="false"

@swernli Thoughts?

`

@swernli
Copy link
Collaborator

swernli commented Dec 6, 2024

I think the right fix would be to enlighten the lint to make it ignore lambdas entirely.

Hmm, this gets tricky. I am not sure if Q# HIR distinguishes between the lamda and callables. I would prefer for users to have a consistent experience when declaring callables and lambdas. This discussion gives me new ideas like introduce linter properties:

  • excludeLambdas=false/true: Whether to exlude lambdas for consideration? Default="false"
  • AggressiveChecking: whether to look out for misusage of NoOp and I. Default="false"

@swernli Thoughts?

I think making it configurable could be good approach, but I think very few folks are aware of the configurability of lints as it stands, so we'd want to make the default behavior something that we think is optimal for the majority of users. Regarding telling the difference between normal callables and those automatically created from lifting lambdas, I've been looking into that for other reasons and considering a change to support it. Currently, every lifted lambda callable is given the name "lambda" in the HIR, which does allow distinguishing them somewhat, but could collide with a normal, user-made callable also named "lambda" in their code. So, one thought I had was updating this line:

name: "lambda".into(),

to set the name of auto-generated lambdas to something that can't normally be used as an identifier name, like "/lambda" or "" and then we can know for certain it's an auto-generated name and not something from user code. Given that this is the second time in a week this has looked like it would help, I'll go ahead and put up a quick PR with that change :)

@swernli
Copy link
Collaborator

swernli commented Dec 6, 2024

Created #2053 for the lambda differentiation question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants