[Bug]: Conditional type aliases treat complete types as generic #26140
Labels
Comments
bradcray
added a commit
that referenced
this issue
Oct 31, 2024
…nd rename `LayoutCS` module (#26137) [reviewed by @ShreyasKhandekar ] As part of our ongoing conversion of domain maps from classes to records, this PR changes the `CS` class that has been used to represent sparse CSR/CSC domains and arrays into a pair of records, `csrLayout` and `cscLayout`. Simultaneously, it renames the `LayoutCS.chpl` module to `CompressedSparseLayout.chpl` to (a) move `Layout` from a prefix to a suffix, similar to the other domain maps and (b) make the name a bit more self-descriptive (since 'CS' isn't generally recognized as being the prefix of CSR/CSC). This approach also made it easy to generate reasonable deprecation warnings for the old approach by essentially starting from a code clone of the LayoutCS module and attaching `@deprecated` attributes to its module and class declarations. Despite these improvements, I've left the new module and record type as `@unstable` since sparse as a whole is unstable and the routines and identifiers used here haven't been reviewed in much detail (though the record names were OK'd by an ad hoc design subteam who wrestled with the question). With this change, I believe `DefaultDist()` is the only class-based domain map remaining and the only thing standing in the way of removing the `dmap` type (?). Most of the diff here is updating module and test code from `dmapped new dmap(new CS())` to one of `dmapped new csrLayout()` or `dmapped new cscLayout()`, preserving the `sortedIndices` argument when used. For the implementation of the record itself, I took a similar approach to the records defining blockDist, cyclicDist, etc. by preserving the old classes and simply wrapping them in the record, combined with using a few helper records—one designed as a means of providing common capabilities between the csrLayout/cscLayout record types and, hopefully, `DefaultDist()` once it's converted to a record; the other (`csLayout`) providing common code between csr/cscLayout since they're basically just transposes of one another. As a result, CompressedSparseLayout.chpl is essentially: * a code clone of LayoutCS.chpl * promoted the module from a `prototype` to a true module by adding `throws` decorators to the `dsiSerialWrite()` calls * added better documentation for the module relative to what we had before * fixed the module's indentation * renamed `LayoutCSDefaultToSorted` to `csLayoutSortByDefault` to better math the module name * updated the `isCSType()` query to work with the new records * changed the `CS` class into a `CSImpl` class that the records wrap * removed its default for `compressRows` in the process because it's unnecessary and could just mask mistakes * added a `chpl_layoutHelper` record that is similar to the `chpl_distHelp` record used for the Block, Cyclic, etc. conversions, whose goal is to factor things that used to be inherited from `BaseDist` into a common record that could be used by multiple types; currently it's only this one, but maybe `DefaultDist` will be able to make use of it as well? Or maybe it's overkill / putting the cart before the horse. * added the `csLayout` record, which simply defines initializers and `==` / `!=` operators * added the `csrLayout` / `cscLayout` records, which simply define initializers * added a `dsiGetDist()` call to convert a class back into a distribution record value Taking stock of other changes required: * modules/layouts/LayoutCS.chpl: marked the module and `CS` class as deprecated * modules/Makefile: added new entry to ensure that the new module is documented * dists/BlockDist.chpl, dists/SparseBlockDist.chpl, packages/LinearAlgebra.chpl: updated to use the new module and type names * test/*.chpl: For all tests, I essentially converted: * uses/imports of `LayoutCS` into `CompressedSparseLayout` * `dmap(new CS())` and `dmap(new CS(compressRows=true))` into `csrLayout()` * `dmap(new CS(compressRows=false))` into `cscLayout()` * in cases like the above that passed `sortIndices`, I preserved those arguments * references to `unmanaged CS` into `csrLayout` * cases that chose between the two using a param variable into a conditional that chooses between the types * cases that have to support both `cs*Layout` and `DefaultDist` to wrap the latter in a dmap+unmanaged and the former not * cases that extended the `CS` class to extend `CSImpl` instead * test/*.compopts: For cases that passed in `CS` as a `config type`, I changed to `csrLayout` In terms of new tests, I: * added a test of the deprecation messages in `test/deprecated/testLayoutCS.chpl` * added a test to make sure the new types worked as expected w.r.t. using parens or not in `test/sparse/CS/cscTypes.chpl` and `test/sparse/CS/csrTypes.chpl` (where I thought that this wasn't working at some point in the past during this effort) And I added some futures for things that surprised me or seemed wrong as I was developing all of the above (some of which were resolved before this was even merged): * #26142: `test/types/records/generic/dispatchWithinParamLoop.chpl` demonstrates that resolution within a param loop over a bool range is not working as expected (and `test/types/records/generic/dispatchWithinIntParamLoop.chpl` as a workaround that works as expected) * #26140 `test/types/records/generic/typeAliasFullyDefaultedGeneric.chpl`: shows that declaring a type alias of two complete types is treated as a generic type, incorrectly I would argue * #26141: `test/types/records/generic/typeAliasFullyDefaultedGeneric2.chpl`: shows that printing out a generic type doesn't seem to give an intuitive result Resolves #25819
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary of Problem
Given a fully-defaulted generic type, like:
we typically treat
Ras a "complete" type, which is to say, nothing more is unbound or generic about it, equivalent toR(42). However, today I was finding that when declaring a type alias with a conditional, like:the compiler would treat
tas generic. Meanwhile, for a simple case like:the compiler treated
tas non-generic (i.e., simply using a type alias to refer to the complete type did not seem to be the issue).Steps to Reproduce
Source Code:
Associated Future Test(s):
test/types/records/generic/typeAliasFullyDefaultedGeneric.chpl#26137Configuration Information
chpl --version: `chpl version 2.2.0The text was updated successfully, but these errors were encountered: