-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Remove the usage of reflection to be trim and aot friendly #1465
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
base: master
Are you sure you want to change the base?
Conversation
|
@tonyqus Looking through the types of relations I didn't find relations with argument POIXMLDocumentPart parent constructors. Did I miss something or this type of constructor is never used? upd: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the usage of reflection for constructing document parts in order to support trimming and AOT (ahead-of-time) compilation. The key changes include replacing reflection-based constructor invocations with delegate-based instantiation, adjusting constructor visibility (from protected to internal), and updating lookup and instance creation patterns across various OOXML classes.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ooxml/XWPF/Usermodel/XWPFRelation.cs | Added extra delegate parameters (often set to null) on relation constructors to replace reflection calls. |
| ooxml/XWPF/Usermodel/XWPFPictureData.cs | Changed the constructor from protected to internal to improve trim friendliness. |
| ooxml/XWPFFactory.cs and ooxml/XSSF/UserModel/*.cs | Updated static instance declarations and relation lookup/refactoring using TryGetValue. |
| ooxml/POIXMLRelation.cs & POIXMLFactory.cs | Replaced reflection-based object creation with delegate invocations and simplified the lookup instantiation logic. |
| Other files | Minor adjustments for consistency in API design and accessibility across OOXML components. |
Comments suppressed due to low confidence (3)
ooxml/XWPF/Usermodel/XWPFPictureData.cs:61
- Changing the constructor from protected to internal alters the API accessibility; please verify that this change does not affect any intended subclassing or external usage.
+ internal XWPFPictureData()
ooxml/POIXMLFactory.cs:57
- Ensure that all relation descriptors have proper delegate implementations provided; otherwise, the fallback to throwing an exception may lead to runtime failures if any delegate remains null.
+ return descriptor.CreatePartWithParent?.Invoke(parent, part) ?? descriptor.CreatePart?.Invoke(part) ?? throw new POIXMLException(new MissingMethodException(descriptor.RelationClass?.Name ?? "Unknown type"));
ooxml/XSSF/UserModel/XSSFRelation.cs:504
- Refactoring the relation lookup to use TryGetValue is a useful improvement; ensure that this pattern is consistently applied throughout the codebase for similar lookups.
+ if (_table.TryGetValue(rel, out XSSFRelation result))
| null | ||
| , null | ||
| , null | ||
| , null | ||
| ); | ||
| public static XWPFRelation TEMPLATE = new XWPFRelation( | ||
| "application/vnd.openxmlformats-officedocument.wordprocessingml.template.main+xml", | ||
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | ||
| "/word/document.xml", | ||
| null | ||
| , null | ||
| , null | ||
| , null | ||
| ); | ||
| public static XWPFRelation MACRO_DOCUMENT = new XWPFRelation( | ||
| "application/vnd.ms-word.document.macroEnabled.main+xml", | ||
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | ||
| "/word/document.xml", | ||
| null | ||
| , null | ||
| , null | ||
| , null | ||
| ); | ||
| public static XWPFRelation MACRO_TEMPLATE_DOCUMENT = new XWPFRelation( | ||
| "application/vnd.ms-word.template.macroEnabledTemplate.main+xml", | ||
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | ||
| "/word/document.xml", | ||
| null | ||
| , null | ||
| , null | ||
| , null | ||
| ); | ||
| public static XWPFRelation GLOSSARY_DOCUMENT = new XWPFRelation( | ||
| "application/vnd.openxmlformats-officedocument.wordprocessingml.document.glossary+xml", | ||
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/glossaryDocument", | ||
| "/word/glossary/document.xml", | ||
| null | ||
| , null | ||
| , null | ||
| , null |
Copilot
AI
Jun 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider providing explicit delegate implementations for relation instantiation where applicable instead of defaulting to null, to ensure clarity and maintainability in the instantiation logic.
| null | |
| , null | |
| , null | |
| , null | |
| ); | |
| public static XWPFRelation TEMPLATE = new XWPFRelation( | |
| "application/vnd.openxmlformats-officedocument.wordprocessingml.template.main+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | |
| "/word/document.xml", | |
| null | |
| , null | |
| , null | |
| , null | |
| ); | |
| public static XWPFRelation MACRO_DOCUMENT = new XWPFRelation( | |
| "application/vnd.ms-word.document.macroEnabled.main+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | |
| "/word/document.xml", | |
| null | |
| , null | |
| , null | |
| , null | |
| ); | |
| public static XWPFRelation MACRO_TEMPLATE_DOCUMENT = new XWPFRelation( | |
| "application/vnd.ms-word.template.macroEnabledTemplate.main+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | |
| "/word/document.xml", | |
| null | |
| , null | |
| , null | |
| , null | |
| ); | |
| public static XWPFRelation GLOSSARY_DOCUMENT = new XWPFRelation( | |
| "application/vnd.openxmlformats-officedocument.wordprocessingml.document.glossary+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/glossaryDocument", | |
| "/word/glossary/document.xml", | |
| null | |
| , null | |
| , null | |
| , null | |
| (part) => null | |
| , () => null | |
| , (part) => null | |
| , () => null | |
| ); | |
| public static XWPFRelation TEMPLATE = new XWPFRelation( | |
| "application/vnd.openxmlformats-officedocument.wordprocessingml.template.main+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | |
| "/word/document.xml", | |
| (part) => null | |
| , () => null | |
| , (part) => null | |
| , () => null | |
| ); | |
| public static XWPFRelation MACRO_DOCUMENT = new XWPFRelation( | |
| "application/vnd.ms-word.document.macroEnabled.main+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | |
| "/word/document.xml", | |
| (part) => null | |
| , () => null | |
| , (part) => null | |
| , () => null | |
| ); | |
| public static XWPFRelation MACRO_TEMPLATE_DOCUMENT = new XWPFRelation( | |
| "application/vnd.ms-word.template.macroEnabledTemplate.main+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument", | |
| "/word/document.xml", | |
| (part) => null | |
| , () => null | |
| , (part) => null | |
| , () => null | |
| ); | |
| public static XWPFRelation GLOSSARY_DOCUMENT = new XWPFRelation( | |
| "application/vnd.openxmlformats-officedocument.wordprocessingml.document.glossary+xml", | |
| "http://schemas.openxmlformats.org/officeDocument/2006/relationships/glossaryDocument", | |
| "/word/glossary/document.xml", | |
| (part) => null | |
| , () => null | |
| , (part) => null | |
| , () => null |
No direct constructors calls is bad for trim analyzers and they are trimmed out
This PR should allow the trim to keep constructors from trimming
Closes issue: #957