Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bamarsha
Copy link
Contributor

@bamarsha bamarsha commented Mar 2, 2021

  • Add parsing for empty array literals.
  • Support sized array literals in attribute arguments.
  • Replace warning when a type parameter doesn't occur in the argument with a warning when the type parameter is unused (in both the argument and the return types).
  • Add tests for integral, iterable, numeric, semigroup, and unwrap constraints.

@bamarsha bamarsha mentioned this pull request Apr 8, 2021
9 tasks
@bamarsha bamarsha changed the title Hindley-Milner type inference Final touches for Hindley-Milner type inference Apr 8, 2021
@bamarsha bamarsha changed the title Final touches for Hindley-Milner type inference Final touches for HM type inference and QEP 2 Apr 8, 2021
Base automatically changed from samarsha/hm-inference to feature/qep2 April 8, 2021 22:51
@bamarsha bamarsha marked this pull request as ready for review April 8, 2021 22:58
@bamarsha bamarsha requested review from bettinaheim and swernli April 8, 2021 22:58
Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I am looking for an override of OnSizedArray in the QIR emission and don't see it yet. What's the plan for that?
  • Also, in CompilerDataStructures.cs, I see a couple of QsExpressionKindNewArray without a matching QsExpressionKindSizedArray - is there something missing there? In BondSchemaTranslator.cs, e.g. in QsExpressionKindComposition NewArray is listed but not SizedArray. I would also expect "SizedArray" to occur in CompilerObjectTranslator. @cesarzc Could you take a look at the bond schemas to have a second set of eyes on that?
  • In ConstantPropagation.fs in shouldPropagate there is NewArray listed but not SizedArray, same in Evaluation.fs, same in HelperFunctions.fs in isLiteral
  • In NamespaceManager.fs in ExpressionHash I would also expect to find SizedArray
  • There is also an overload missing in QsharpCodeOutput.cs

Also, is there a matching PR on the runtime repo to add support in the C# generation?
Edit: I reviewed (i.e. set changes requested) in the same way as I would for a PR to main - since this is going into a feature branch, I am happy to remove the blocker to split it into several PRs if you prefer. If you want to do that, then I suggest to make a task list in a GitHub issue to make sure the remaining work is tracked and not forgotten.

@bamarsha
Copy link
Contributor Author

bamarsha commented Apr 9, 2021

  • I am looking for an override of OnSizedArray in the QIR emission and don't see it yet. What's the plan for that?
  • Also, in CompilerDataStructures.cs, I see a couple of QsExpressionKindNewArray without a matching QsExpressionKindSizedArray - is there something missing there? In BondSchemaTranslator.cs, e.g. in QsExpressionKindComposition NewArray is listed but not SizedArray. I would also expect "SizedArray" to occur in CompilerObjectTranslator. @cesarzc Could you take a look at the bond schemas to have a second set of eyes on that?
  • In ConstantPropagation.fs in shouldPropagate there is NewArray listed but not SizedArray, same in Evaluation.fs, same in HelperFunctions.fs in isLiteral
  • In NamespaceManager.fs in ExpressionHash I would also expect to find SizedArray
  • There is also an overload missing in QsharpCodeOutput.cs

Also, is there a matching PR on the runtime repo to add support in the C# generation?

I don't think any of these issues are new in this PR. The SizedArray expression kind was added in #856, with corresponding C# generation added in microsoft/qsharp-runtime#507.

@bettinaheim
Copy link
Contributor

  • I am looking for an override of OnSizedArray in the QIR emission and don't see it yet. What's the plan for that?
  • Also, in CompilerDataStructures.cs, I see a couple of QsExpressionKindNewArray without a matching QsExpressionKindSizedArray - is there something missing there? In BondSchemaTranslator.cs, e.g. in QsExpressionKindComposition NewArray is listed but not SizedArray. I would also expect "SizedArray" to occur in CompilerObjectTranslator. @cesarzc Could you take a look at the bond schemas to have a second set of eyes on that?
  • In ConstantPropagation.fs in shouldPropagate there is NewArray listed but not SizedArray, same in Evaluation.fs, same in HelperFunctions.fs in isLiteral
  • In NamespaceManager.fs in ExpressionHash I would also expect to find SizedArray
  • There is also an overload missing in QsharpCodeOutput.cs

Also, is there a matching PR on the runtime repo to add support in the C# generation?

I don't think any of these issues are new in this PR. The SizedArray expression kind was added in #856, with corresponding C# generation added in microsoft/qsharp-runtime#507.

See the edit above (you have beaten me to it with your reply):
Edit: I reviewed (i.e. set changes requested) in the same way as I would for a PR to main - since this is going into a feature branch, I am happy to remove the blocker to split it into several PRs if you prefer. If you want to do that, then I suggest to make a task list in a GitHub issue to make sure the remaining work is tracked and not forgotten.

I am listing the things that are missing for the feature to be complete, since this PR is titled "Final touches for ... QEP 2"; up to you when to address them as long as they are done before the merge to main. Could you please track the items I listed somewhere, if they are not addressed with this PR?

@bamarsha bamarsha changed the title Final touches for HM type inference and QEP 2 Miscellaneous changes for HM type inference and QEP 2 Apr 9, 2021
@bamarsha
Copy link
Contributor Author

bamarsha commented Apr 9, 2021

Sorry, the name is misleading. I didn't intend for this to be the final PR.

@bamarsha
Copy link
Contributor Author

bamarsha commented Apr 9, 2021

Also, in CompilerDataStructures.cs, I see a couple of QsExpressionKindNewArray without a matching QsExpressionKindSizedArray - is there something missing there?

That's intentional. SizedArray uses QsExpressionKindExpressionDouble.

In BondSchemaTranslator.cs, e.g. in QsExpressionKindComposition NewArray is listed but not SizedArray. I would also expect "SizedArray" to occur in CompilerObjectTranslator.

Also intentional, due to piggy-backing on the existing QsExpressionKindExpressionDouble.

@cesarzc Could you take a look at the bond schemas to have a second set of eyes on that?

@cesarzc already reviewed the changes to the Bond schema in #856.

@bamarsha
Copy link
Contributor Author

bamarsha commented Apr 9, 2021

I am looking for an override of OnSizedArray in the QIR emission and don't see it yet. What's the plan for that?

Keep in mind that SizedArray was added before QIR generation was even in main. :) It should be added before merging the feature branch, though, yes.

@bettinaheim bettinaheim dismissed their stale review April 9, 2021 00:57

Remaining work is tracked in #857

@bamarsha bamarsha merged commit ef2a456 into feature/qep2 Apr 9, 2021
@bamarsha bamarsha deleted the samarsha/hm branch April 9, 2021 18:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants